paulkirth added a comment.
@brettw Thanks for the clarification. While I'm more familiar w/ these set of
patches, I have to say that that I was also unsure/surprised by some of these
changes. If you have concrete plans for cleanups letting us know about those
plans in the initial review will help things proceed more smoothly. For
instance, I wasn't aware that you had plans to refactor the various `TypeInfos`
until now, and that context will make reviewing new patches more
straightforward on my end.
Anyway, thanks for taking the time to provide the full context for us. I found
it very helpful.
================
Comment at: clang-tools-extra/clang-doc/Representation.h:165
- : Type(Type, Field, IT) {}
- TypeInfo(SymbolID Type, StringRef Field, InfoType IT, StringRef Path)
- : Type(Type, Field, IT, Path) {}
----------------
brettw wrote:
> haowei wrote:
> > The clean up here prevents setting a customized SymbolID for TypeInfo,
> > which becomes a problem in the HTMLGeneratorTest, in which TypeInfo is
> > constructed with an constant EmptySID. While the outcome is the same, it is
> > not semantically equivalent.
> It does not prevent setting a custom SymbolID. The full constructor is the
> one above that takes a Reference. That is the one that's normally used in the
> production code; I added it in my previous patch. This patch is a followup
> from that one because I wanted to remove some now-redundant constructors but
> thought that I would separate it out to make it easier to review. The
> minority of cases in HTMLGeneratorTest that needed to pass more than the
> simple name/path have been updated to take a Reference.
>
> The only code that changed in a way that you described is on line 373. But
> I'm not sure what you're arguing about "semantically equivalent". In this
> case, the caller wanted a "void" TypeInfo but tryped a bunch of unnecessary
> stuff. I think it's likely that the author of this code was confused about
> the constructor situation for TypeInfo and used a more complex variant than
> was required. It illustrates exactly why this cleanup is useful.
> It does not prevent setting a custom SymbolID. The full constructor is the
> one above that takes a Reference. That is the one that's normally used in the
> production code; I added it in my previous patch.
I think the more salient point is that the SymbolID is only used in TypeInfo to
instantiate a `Reference`, so I don't think we're losing too much flexibility
by forcing users of this API to construct the `Reference` directly.
If we want to leave support for setting the SymbolD in place, we should mark it
with a FIXME and an issue on github to revisit. Otherwise, I think we can move
on.
> This patch is a followup from that one because I wanted to remove some
> now-redundant constructors but thought that I would separate it out to make
> it easier to review.
Thank you. Small incremental patches are always appreciated.
>The minority of cases in HTMLGeneratorTest that needed to pass more than the
>simple name/path have been updated to take a Reference.
>
> The only code that changed in a way that you described is on line 373. But
> I'm not sure what you're arguing about "semantically equivalent".
I agree w/ @haowei that these are not semantically equivalent, despite the fact
that they result in the same output. EmptySID is const and is a specific
instance of SymbolID that has a unique meaning. While it is just default
constructed, using another instance is semantically different, regardless if
the resulting output will be the same or not.
>In this case, the caller wanted a "void" TypeInfo but tryped a bunch of
>unnecessary stuff. I think it's likely that the author of this code was
>confused about the constructor situation for TypeInfo and used a more complex
>variant than was required. It illustrates exactly why this cleanup is useful.
Let's leave speculation about the original authors out of this review. we all
agree that clang-doc is in need of maintenance, so thank you for your patch.
================
Comment at: clang-tools-extra/clang-doc/Representation.h:202
+ bool IsFileInRootDir = false)
: LineNumber(LineNumber), Filename(std::move(Filename)),
IsFileInRootDir(IsFileInRootDir) {}
----------------
do we still want to use `std::move` here now that its a StringRef? I don't
think we would do that w/ `const char*`, which is what `StringRef` boils down
to. It should be passed by value everywhere.
see
https://llvm.org/docs/ProgrammersManual.html#passing-strings-the-stringref-and-twine-classes,
https://llvm.org/docs/ProgrammersManual.html#the-stringref-class
https://llvm.org/docs/ProgrammersManual.html#string-like-containers
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134235/new/
https://reviews.llvm.org/D134235
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
