[PATCH] D134235: [clang-doc] Clean up *Info constructors.

[Paul Kirth via Phabricator via cfe-commits]

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