paulkirth accepted this revision.
paulkirth added a comment.
This revision is now accepted and ready to land.
This is basically LGTM. There are a few comments I think could be phrased a bit
more clearly, but otherwise are more or less fine.
================
Comment at: clang-tools-extra/clang-doc/Representation.h:119-123
+ // This variant (that takes no qualified name parameter) uses the Name as the
+ // QualName (very useful in unit tests to reduce verbosity). This can't use
+ // the empty string as a default because the global namespace uses
+ // "GlobalNamespace" as the name, but should have an empty QualName
+ // (corresponding to the name in code).
----------------
Does this imply that its illegal to use `""` as the name with this constructor?
if so, it probably requires at least an assert.
================
Comment at: clang-tools-extra/clang-doc/Representation.h:200-202
+// is quite involved. Currently the documentation uses of this data just
involve
+// echoing it back to reconstruct the declaration of the class or function, so
+// the full contents is sufficient.
----------------
I found this comment a bit hard to parse. Do you think this is a good way to
rephrase?
================
Comment at: clang-tools-extra/clang-doc/Representation.h:210-212
+ // The literal contents of the code for everything that specifies this
+ // template parameter. This will include all components, so for a template
+ // declaration it could be "typename T = int".
----------------
I think this is equivalent. Do you mean this contains //all// instantiations of
the parameter across the codebase? or for a single declaration?
================
Comment at: clang-tools-extra/clang-doc/Representation.h:224-225
+
+// This struct will be a member of the record/function that is the template or
+// specialization.
+struct TemplateInfo {
----------------
Can you rephrase this? I think you're saying this type contains template
information for a function or type that contains a template or template
specialization. Is that correct?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D139154/new/
https://reviews.llvm.org/D139154
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
