Template diffing: Type eliding and tree printing

[chandlerc, 2011-12-13 22:24:01]

http://codereview.appspot.com/5487054/diff/1/include/clang/AST/Type.h
File include/clang/AST/Type.h (right):

http://codereview.appspot.com/5487054/diff/1/include/clang/AST/Type.h#newcode...
include/clang/AST/Type.h:4781: /// Insertion operator for diagnostics.  This
allows sending QualType's into a
This comment needs updating -- it's for a pair of qual types.

http://codereview.appspot.com/5487054/diff/1/include/clang/AST/Type.h#newcode...
include/clang/AST/Type.h:4790: /// Insertion operator for partial diagnostics. 
This allows sending QualType's
Ditto.

http://codereview.appspot.com/5487054/diff/1/include/clang/Basic/Diagnostic.h
File include/clang/Basic/Diagnostic.h (right):

http://codereview.appspot.com/5487054/diff/1/include/clang/Basic/Diagnostic.h...
include/clang/Basic/Diagnostic.h:136: ak_qualtype_pair    // QualType
Comment update

http://codereview.appspot.com/5487054/diff/1/include/clang/Basic/Diagnostic.h...
include/clang/Basic/Diagnostic.h:160: bool ShowColors;               // Color
printing is enabled.
Why is this here instead of DiagnosticOptions?

http://codereview.appspot.com/5487054/diff/1/include/clang/Basic/Diagnostic.h...
include/clang/Basic/Diagnostic.h:731: diagObj->ToType = 0;
These seem a bit out of place... Shouldn't something else be clearing the other
fields already?

http://codereview.appspot.com/5487054/diff/1/include/clang/Basic/PartialDiagn...
File include/clang/Basic/PartialDiagnostic.h (right):

http://codereview.appspot.com/5487054/diff/1/include/clang/Basic/PartialDiagn...
include/clang/Basic/PartialDiagnostic.h:71: intptr_t FromType, ToType;
Doxygen comment for these?

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp
File lib/AST/ASTDiagnostic.cpp (right):

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:230: static bool RunDiff(ASTContext &Context, QualType
FromType, QualType ToType,
A better name would help me here. FormatTemplateTypeDiff? Dunno.

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:259: bool ShowColors = (Val & 8) == 8;
Yikes, how do we get this mapping? Why can't we pull in some header or enum to
interpret these options? I find this quite magical...

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:261:
QualType::getFromOpaquePtr(reinterpret_cast<void*>(QualTypeVals[0]));
I find it a bit odd that we pull the types out of the QualTypeVals -- how do we
know which entries to look at?

I'm imagining a diagnostic which wants to first print a QualType, and then wants
to diff two other QualTypes. It seems like we should harden against that.

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:265: NeedQuotes = false;
? Why set this to false (as opposed to just ignore it)?

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:275: // Type diffing failed.  Fall through to regular
type printing.
Do we only print one of the types in this case? Do we print both?

In what cases does this fail? I feel like this could use some beefier comments
to explain exactly what is expected to happen at this point.

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:367: // two template types are compared.
Doxygen comments please. Also, it would be good to really flesh this comment
out. Give an overview of how the class works, what the algorithm is, what the
various features are, etc.

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:368: class TemplateDiff {
Need to wrap this in an anonymous namespace.

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:376: unsigned NumElidedTypes;
Please provide doxygen comments for all of the fields.

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:377: static const char BOLD = 127;
Why all caps? Is there a common header you can get this magic value from?

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:378: TemplateDiff();
Why do we have a default constructor?

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:382: // of the TemplateArguments.
Update all of your class and method comments to be doxygen as well.

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:387: TemplateArgument::pack_iterator endTA;
These names don't follow the coding conventions in Clang.

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:389: TSTiterator(const TemplateSpecializationType
*TST)
Comments on the methods of this iterator please, it's not entirely obvious how
it works (although it looks quite clever!)

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:403: return this->TST == Iter.TST && this->index ==
Iter.index &&
No need to use 'this->'.

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:416: if (isEnd()) return *this;
Shouldn't this just be an assert(!isEnd()); ?

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:448: std::string Bold(std::string S) {
This looks really copy-happy. Have you looked at raw_svector_ostream and
SmallString (which derives from SmallVector)? I think using that combination
would simplify a lot of the code in this class and make it more efficient.

Essentially, in the constructor you could set up the buffer and the stream. As
you build up the type to print, you can stream it into the instance stream. Then
MakeString can render that stream into a std::string.

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:456: // canonical type when the original typenames has
the same name.  If the
s/has/have/

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:457: // canonical type names are also the same, use
the original name.
This looks like duplicate logic from other parts of the diagnostics
infrastructure (the a.k.a. handling). Can we instead re-use the a.k.a. printing
logic here?

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:470: // overly long names.
It would be good to have a fixme to include special logic for function pointer
types.

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:474: FromTypeStr += "<...>";
I thought we wanted '[...]' to be used for elision to avoid confusion with
variadic templates? And '[N x ...]' for eliding N template parameters?

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:503: // Converts an expression parameter into a
string.
Again, it would be good to use existing diagnostic logic for printing an
expresison...

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:543: "<...>";
This seems a bit magiacl... Comment please? Also, I think that the one-line if
with a return after it isn't helping readability givin this long argument.

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:903: public :
?

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:911: S = ""; }
Really? ;] We can make this cleaner...

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:924: }
No curlies here..

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:927: // Top-level templates are different.  Print them
and stop.
What if the arguments are also different? Shouldn't we print those?

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:934: "<...> != " +
What if we're not eliding?

http://codereview.appspot.com/5487054/diff/1/lib/Driver/Tools.cpp
File lib/Driver/Tools.cpp (right):

http://codereview.appspot.com/5487054/diff/1/lib/Driver/Tools.cpp#newcode1867
lib/Driver/Tools.cpp:1867: CmdArgs.push_back("-fno-elide-type");
I would just add the last of these two flags to CmdArgs, w/o any default logic
here.

Probably do the same thing for the tree printing flag as well.

http://codereview.appspot.com/5487054/diff/1/lib/Frontend/CompilerInvocation.cpp
File lib/Frontend/CompilerInvocation.cpp (right):

http://codereview.appspot.com/5487054/diff/1/lib/Frontend/CompilerInvocation....
lib/Frontend/CompilerInvocation.cpp:1220: Opts.ElideType = false;
You should be able to use the hasFlag logic here? Specifically, setting the Opts
field to the hasFlag result?

http://codereview.appspot.com/5487054/diff/1/lib/Frontend/TextDiagnostic.cpp
File lib/Frontend/TextDiagnostic.cpp (right):

http://codereview.appspot.com/5487054/diff/1/lib/Frontend/TextDiagnostic.cpp#...
lib/Frontend/TextDiagnostic.cpp:42: if (Str[i] != BOLD) {
Oh wow, I see. You're using 127 as a magic character to enable bolding. I wonder
if there is a better character code for this? It seems... quite subtle.

http://codereview.appspot.com/5487054/diff/1/lib/Sema/SemaOverload.cpp
File lib/Sema/SemaOverload.cpp (right):

http://codereview.appspot.com/5487054/diff/1/lib/Sema/SemaOverload.cpp#newcod...
lib/Sema/SemaOverload.cpp:7432: << std::pair<QualType, QualType>(FromTy, ToTy)
make_pair?

[rtrieu, 2012-01-12 01:08:18]

http://codereview.appspot.com/5487054/diff/1/include/clang/AST/Type.h
File include/clang/AST/Type.h (right):

http://codereview.appspot.com/5487054/diff/1/include/clang/AST/Type.h#newcode...
include/clang/AST/Type.h:4781: /// Insertion operator for diagnostics.  This
allows sending QualType's into a
On 2011/12/13 22:24:01, chandlerc wrote:
> This comment needs updating -- it's for a pair of qual types.

Done.

http://codereview.appspot.com/5487054/diff/1/include/clang/AST/Type.h#newcode...
include/clang/AST/Type.h:4790: /// Insertion operator for partial diagnostics. 
This allows sending QualType's
On 2011/12/13 22:24:01, chandlerc wrote:
> Ditto.

Done.

http://codereview.appspot.com/5487054/diff/1/include/clang/Basic/Diagnostic.h
File include/clang/Basic/Diagnostic.h (right):

http://codereview.appspot.com/5487054/diff/1/include/clang/Basic/Diagnostic.h...
include/clang/Basic/Diagnostic.h:136: ak_qualtype_pair    // QualType
On 2011/12/13 22:24:01, chandlerc wrote:
> Comment update

Done.

http://codereview.appspot.com/5487054/diff/1/include/clang/Basic/Diagnostic.h...
include/clang/Basic/Diagnostic.h:160: bool ShowColors;               // Color
printing is enabled.
On 2011/12/13 22:24:01, chandlerc wrote:
> Why is this here instead of DiagnosticOptions?

It is in both locations.  I wasn't sure DiagnosticsOptions would be available
when needed, so I put them here as well.

http://codereview.appspot.com/5487054/diff/1/include/clang/Basic/Diagnostic.h...
include/clang/Basic/Diagnostic.h:731: diagObj->ToType = 0;
On 2011/12/13 22:24:01, chandlerc wrote:
> These seem a bit out of place... Shouldn't something else be clearing the
other
> fields already?
The data in the old DiagnosticEngine is not cleared when it is passed in.  For
the arrays, simply setting the index to 0 will allow new data to overwrite the
old data, but this trick doesn't work with FromType and ToType so they must be
explicitly cleared here.

http://codereview.appspot.com/5487054/diff/1/include/clang/Basic/PartialDiagn...
File include/clang/Basic/PartialDiagnostic.h (right):

http://codereview.appspot.com/5487054/diff/1/include/clang/Basic/PartialDiagn...
include/clang/Basic/PartialDiagnostic.h:71: intptr_t FromType, ToType;
On 2011/12/13 22:24:01, chandlerc wrote:
> Doxygen comment for these?

Done.

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp
File lib/AST/ASTDiagnostic.cpp (right):

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:230: static bool RunDiff(ASTContext &Context, QualType
FromType, QualType ToType,
On 2011/12/13 22:24:01, chandlerc wrote:
> A better name would help me here. FormatTemplateTypeDiff? Dunno.

Done.

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:259: bool ShowColors = (Val & 8) == 8;
On 2011/12/13 22:24:01, chandlerc wrote:
> Yikes, how do we get this mapping? Why can't we pull in some header or enum to
> interpret these options? I find this quite magical...
Thrown the magic values into an enum in Diagnostic.h

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:261:
QualType::getFromOpaquePtr(reinterpret_cast<void*>(QualTypeVals[0]));
On 2011/12/13 22:24:01, chandlerc wrote:
> I find it a bit odd that we pull the types out of the QualTypeVals -- how do
we
> know which entries to look at?
> 
> I'm imagining a diagnostic which wants to first print a QualType, and then
wants
> to diff two other QualTypes. It seems like we should harden against that.

When QualType's are printed (Kind = ak_qualtype), a QualType pointer is stored
in Val and QualTypeVals are used to determine AKA.

When a type diff is needed, two QualTypes are needed, which won't fit into Val,
and several other flags are needed to be passed it.  Val is re-purposed as a bit
field and the two QualTypes are stored as the first values of QualTypeVals.

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:265: NeedQuotes = false;
On 2011/12/13 22:24:01, chandlerc wrote:
> ? Why set this to false (as opposed to just ignore it)?

This has been changed to be set if the type diff was successful.  The tree diff
does not need it, but the single type printing does.

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:275: // Type diffing failed.  Fall through to regular
type printing.
On 2011/12/13 22:24:01, chandlerc wrote:
> Do we only print one of the types in this case? Do we print both?
> 
Only one type is printed.

> In what cases does this fail? I feel like this could use some beefier comments
> to explain exactly what is expected to happen at this point.

Comments updated.  If the types are not templates, or the templates are
different, use the regular type printing.

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:367: // two template types are compared.
On 2011/12/13 22:24:01, chandlerc wrote:
> Doxygen comments please. Also, it would be good to really flesh this comment
> out. Give an overview of how the class works, what the algorithm is, what the
> various features are, etc.

Done.

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:368: class TemplateDiff {
On 2011/12/13 22:24:01, chandlerc wrote:
> Need to wrap this in an anonymous namespace.

Done.

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:376: unsigned NumElidedTypes;
On 2011/12/13 22:24:01, chandlerc wrote:
> Please provide doxygen comments for all of the fields.

Done.

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:377: static const char BOLD = 127;
On 2011/12/13 22:24:01, chandlerc wrote:
> Why all caps? Is there a common header you can get this magic value from?

Done.

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:378: TemplateDiff();
On 2011/12/13 22:24:01, chandlerc wrote:
> Why do we have a default constructor?

Done.

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:382: // of the TemplateArguments.
On 2011/12/13 22:24:01, chandlerc wrote:
> Update all of your class and method comments to be doxygen as well.

Done.

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:387: TemplateArgument::pack_iterator endTA;
On 2011/12/13 22:24:01, chandlerc wrote:
> These names don't follow the coding conventions in Clang.

Done.

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:389: TSTiterator(const TemplateSpecializationType
*TST)
On 2011/12/13 22:24:01, chandlerc wrote:
> Comments on the methods of this iterator please, it's not entirely obvious how
> it works (although it looks quite clever!)

Done.

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:403: return this->TST == Iter.TST && this->index ==
Iter.index &&
On 2011/12/13 22:24:01, chandlerc wrote:
> No need to use 'this->'.

Done.

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:416: if (isEnd()) return *this;
On 2011/12/13 22:24:01, chandlerc wrote:
> Shouldn't this just be an assert(!isEnd()); ?

Two TSTiterator's are used to traverse the template arguments of the two types. 
In case one type has more arguments than the other, make the increment operator
a no-op.

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:448: std::string Bold(std::string S) {
On 2011/12/13 22:24:01, chandlerc wrote:
> This looks really copy-happy. Have you looked at raw_svector_ostream and
> SmallString (which derives from SmallVector)? I think using that combination
> would simplify a lot of the code in this class and make it more efficient.
> 
> Essentially, in the constructor you could set up the buffer and the stream. As
> you build up the type to print, you can stream it into the instance stream.
Then
> MakeString can render that stream into a std::string.

Done.

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:456: // canonical type when the original typenames has
the same name.  If the
On 2011/12/13 22:24:01, chandlerc wrote:
> s/has/have/

Done.

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:474: FromTypeStr += "<...>";
On 2011/12/13 22:24:01, chandlerc wrote:
> I thought we wanted '[...]' to be used for elision to avoid confusion with
> variadic templates? And '[N x ...]' for eliding N template parameters?

Done.

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:503: // Converts an expression parameter into a
string.
On 2011/12/13 22:24:01, chandlerc wrote:
> Again, it would be good to use existing diagnostic logic for printing an
> expresison...

Done.  See PrintExpr

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:543: "<...>";
On 2011/12/13 22:24:01, chandlerc wrote:
> This seems a bit magiacl... Comment please? Also, I think that the one-line if
> with a return after it isn't helping readability givin this long argument.

Removed.

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:903: public :
On 2011/12/13 22:24:01, chandlerc wrote:
> ?

Demarcation between internally visible above and externally visible below.

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:911: S = ""; }
On 2011/12/13 22:24:01, chandlerc wrote:
> Really? ;] We can make this cleaner...

Done.

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:924: }
On 2011/12/13 22:24:01, chandlerc wrote:
> No curlies here..

Done.

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:927: // Top-level templates are different.  Print them
and stop.
On 2011/12/13 22:24:01, chandlerc wrote:
> What if the arguments are also different? Shouldn't we print those?

Eliding is not longer done in this case.  In fact, diffing stops in this case
and type printing is handed by the default type printing.

http://codereview.appspot.com/5487054/diff/1/lib/AST/ASTDiagnostic.cpp#newcod...
lib/AST/ASTDiagnostic.cpp:934: "<...> != " +
On 2011/12/13 22:24:01, chandlerc wrote:
> What if we're not eliding?

Eliding is no longer done in this case.

http://codereview.appspot.com/5487054/diff/1/lib/Driver/Tools.cpp
File lib/Driver/Tools.cpp (right):

http://codereview.appspot.com/5487054/diff/1/lib/Driver/Tools.cpp#newcode1867
lib/Driver/Tools.cpp:1867: CmdArgs.push_back("-fno-elide-type");
On 2011/12/13 22:24:01, chandlerc wrote:
> I would just add the last of these two flags to CmdArgs, w/o any default logic
> here.
> 
> Probably do the same thing for the tree printing flag as well.

Done.

http://codereview.appspot.com/5487054/diff/1/lib/Frontend/CompilerInvocation.cpp
File lib/Frontend/CompilerInvocation.cpp (right):

http://codereview.appspot.com/5487054/diff/1/lib/Frontend/CompilerInvocation....
lib/Frontend/CompilerInvocation.cpp:1220: Opts.ElideType = false;
On 2011/12/13 22:24:01, chandlerc wrote:
> You should be able to use the hasFlag logic here? Specifically, setting the
Opts
> field to the hasFlag result?

Done.

http://codereview.appspot.com/5487054/diff/1/lib/Frontend/TextDiagnostic.cpp
File lib/Frontend/TextDiagnostic.cpp (right):

http://codereview.appspot.com/5487054/diff/1/lib/Frontend/TextDiagnostic.cpp#...
lib/Frontend/TextDiagnostic.cpp:42: if (Str[i] != BOLD) {
On 2011/12/13 22:24:01, chandlerc wrote:
> Oh wow, I see. You're using 127 as a magic character to enable bolding. I
wonder
> if there is a better character code for this? It seems... quite subtle.

The bold character has been moved to shared header.  Several other character
codes are unused character codes that may be used for this.  There's no
particular attachment for 127.

http://codereview.appspot.com/5487054/diff/1/lib/Sema/SemaOverload.cpp
File lib/Sema/SemaOverload.cpp (right):

http://codereview.appspot.com/5487054/diff/1/lib/Sema/SemaOverload.cpp#newcod...
lib/Sema/SemaOverload.cpp:7432: << std::pair<QualType, QualType>(FromTy, ToTy)
On 2011/12/13 22:24:01, chandlerc wrote:
> make_pair?

Done.