Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(230)

Issue 5487054: Template diffing: Type eliding and tree printing

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 3 months ago by rtrieu
Modified:
12 years, 2 months ago
Reviewers:
chandlerc
Base URL:
https://llvm.org/svn/llvm-project/cfe/trunk/
Visibility:
Public.

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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1342 lines, -36 lines) Patch
M include/clang/AST/Type.h View 1 1 chunk +18 lines, -0 lines 0 comments Download
M include/clang/Basic/Diagnostic.h View 1 2 8 chunks +57 lines, -3 lines 0 comments Download
M include/clang/Basic/DiagnosticSemaKinds.td View 1 chunk +3 lines, -3 lines 0 comments Download
M include/clang/Basic/PartialDiagnostic.h View 1 5 chunks +28 lines, -3 lines 0 comments Download
M include/clang/Driver/CC1Options.td View 2 chunks +9 lines, -0 lines 0 comments Download
M include/clang/Driver/Options.td View 1 1 chunk +4 lines, -0 lines 0 comments Download
M include/clang/Frontend/DiagnosticOptions.h View 1 chunk +3 lines, -0 lines 0 comments Download
M lib/AST/ASTDiagnostic.cpp View 1 2 4 chunks +927 lines, -0 lines 0 comments Download
M lib/Basic/Diagnostic.cpp View 1 4 chunks +71 lines, -2 lines 0 comments Download
M lib/Driver/Tools.cpp View 1 2 chunks +11 lines, -0 lines 0 comments Download
M lib/Frontend/CompilerInvocation.cpp View 1 1 chunk +5 lines, -0 lines 0 comments Download
M lib/Frontend/TextDiagnostic.cpp View 1 2 4 chunks +18 lines, -3 lines 0 comments Download
M lib/Frontend/Warnings.cpp View 1 chunk +4 lines, -0 lines 0 comments Download
M lib/Sema/SemaOverload.cpp View 1 1 chunk +1 line, -1 line 0 comments Download
M test/Misc/diag-aka-types.cpp View 1 chunk +0 lines, -21 lines 0 comments Download
A test/Misc/diag-template-diffing.cpp View 1 1 chunk +183 lines, -0 lines 0 comments Download

Messages

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 ...
12 years, 3 months ago (2011-12-13 22:24:01 UTC) #1
rtrieu
12 years, 2 months ago (2012-01-12 01:08:18 UTC) #2
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b