|
|
Patch Set 1 #
Total comments: 72
Patch Set 2 : Separate diffing logic from printing logic #Patch Set 3 : Move the bold toggle character to a shared header. #
Total messages: 2
|
chandlerc
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#newcode4781 include/clang/AST/Type.h:4781: /// Insertion operator for diagnostics. This allows sending QualType's ...
|
14 years ago (2011年12月13日 22:24:01 UTC) #1 | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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?
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.