aaron.ballman added inline comments.
================ Comment at: clang/lib/AST/ASTDumper.cpp:205 + if (const Decl *D = dyn_cast<Decl>(this)) + D->dump(); +} ---------------- shafik wrote: > aaron.ballman wrote: > > shafik wrote: > > > aaron.ballman wrote: > > > > erichkeane wrote: > > > > > aaron.ballman wrote: > > > > > > erichkeane wrote: > > > > > > > One thing to note is that the 'else' case here is a little > > > > > > > uninformative. See > > > > > > > https://clang.llvm.org/doxygen/DeclBase_8cpp_source.html#l00915 > > > > > > > for some similar logic here (though not sure we should be > > > > > > > emulating that). > > > > > > > > > > > > > > More, I wonder if there is SOME message here that should be > > > > > > > dumped for 'else'. > > > > > > Looking at what inherits from `DeclContext`, is there use of it > > > > > > which is *not* a `Decl`? I couldn't find a use where it's not also > > > > > > a `Decl`. > > > > > I've DEFINITELY run into it in the debugger before, but I have no > > > > > idea WHAT case that is. It is sometimes just "DeclContext is an > > > > > invalid pointer" kinda thing, so it might be worth-while to have SOME > > > > > output besides "print nothing", particularly when debugging. > > > > IIRC, the case this comes up in is when the object is only partially > > > > constructed, and so I agree that having an `else` clause here would be > > > > useful -- because this interface is predominately used from a debugger, > > > > it has to deal with special "impossible" situations a bit more > > > > carefully. > > > So looking at the other dump member functions, all of them seem to assume > > > we have a valid `DeclContext` and so they do not have any else either. > > > > > > So maybe we want to think about making the rest a bit smarter as well? > > > So maybe we want to think about making the rest a bit smarter as well? > > > > What do you have in mind? > > > > I think it's plausible to be in a partially constructed state in terms of > > when ctors are called, but I'm far less worried about people calling a dump > > method in the middle of the member inits happening. > > > So maybe we want to think about making the rest a bit smarter as well? > > > > What do you have in mind? > > > > I think it's plausible to be in a partially constructed state in terms of > > when ctors are called, but I'm far less worried about people calling a dump > > method in the middle of the member inits happening. > > Apologies, what I meant was if we are going to add an `else` here then we > should be consistent and add it in the other dump functions as well. None of the other DeclContext dump functions cast to `Decl`, so I'm not certain there's an `else` to add to any of them, or am I misunderstanding? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133499/new/ https://reviews.llvm.org/D133499 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
