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

This revision was automatically updated to reflect the committed changes. Closed by commit rG0afc60858e11: [clang-doc] Clean up *Info constructors. (authored by brettw, committed by paulkirth). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

paulkirth accepted this revision. paulkirth added a comment. I'm good w/ this. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134235/new/ https://reviews.llvm.org/D134235 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

haowei accepted this revision. haowei added a comment. This revision is now accepted and ready to land. LGTM @paulkirth Do you have more comments? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134235/new/ https://reviews.llvm.org/D134235 ___

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

brettw added inline comments. Comment at: clang-tools-extra/clang-doc/BitcodeReader.cpp:336 Reference *I, FieldId ) { switch (ID) { case REFERENCE_USR: haowei wrote: > This block ID came from L582, which read from a bitcode file

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

haowei added inline comments. Comment at: clang-tools-extra/clang-doc/BitcodeReader.cpp:336 Reference *I, FieldId ) { switch (ID) { case REFERENCE_USR: This block ID came from L582, which read from a bitcode file when

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

Please avoid directly replying to the email as they won't be logged in the phabricator. This boolean is logged into YAML output, while I don't have data, I could bet there will be users depending on it even if it looks so unlikely. We don't own the tool in the first place, in that case, I felt if

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

brettw added a comment. I can guarantee that nobody uses this for anything useful because it doesn't work well enough. For example, before I started changing it, requesting HTML output crashed on startup and many of the most basic syntactic things were broken. My previous patch changed enums

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

On Mon, Sep 26, 2022 at 1:42 PM Haowei Wu via Phabricator < revi...@reviews.llvm.org> wrote: > haowei added a comment. > > @brettw Thanks for the clarification. I was oncall for buildgardener last > week and got quite busy due to breakages. > > Thanks for your clarification, that is really

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

haowei added a comment. @brettw Thanks for the clarification. I was oncall for buildgardener last week and got quite busy due to breakages. Thanks for your clarification, that is really helpful to understand the your intent. While I still don't see the way these constructors are implemented is

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

brettw added inline comments. Comment at: clang-tools-extra/clang-doc/Representation.h:202 + bool IsFileInRootDir = false) : LineNumber(LineNumber), Filename(std::move(Filename)), IsFileInRootDir(IsFileInRootDir) {} paulkirth wrote: >

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

brettw updated this revision to Diff 462281. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134235/new/ https://reviews.llvm.org/D134235 Files: clang-tools-extra/clang-doc/BitcodeReader.cpp clang-tools-extra/clang-doc/BitcodeWriter.cpp clang-tools-extra/clang-doc/BitcodeWriter.h

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

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

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

brettw marked an inline comment as done. brettw added a comment. If you have more questions, I can schedule a meeting. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134235/new/ https://reviews.llvm.org/D134235

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

brettw marked an inline comment as done. brettw added a comment. First, for the mechanical part of the change, this code implement multiple constructors:   Info() = default;   Info(InfoType IT) : IT(IT) {}   Info(InfoType IT, SymbolID USR) : USR(USR), IT(IT) {}   Info(InfoType IT, SymbolID USR,

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

haowei added a comment. I am a bit puzzled by the intent of cleaning up the constructors in Info object. I didn't see it as an issue to have multiple constructors, which is a common practice. After this clean up It doesn't make it easier to use these objects, in fact, in `Serialize.cpp`, you

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

brettw created this revision. Herald added a project: All. brettw requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. The *Info object (for the copy of the AST") constructors had many duplicated variants. Many of the variants