jblespiau marked an inline comment as done.
jblespiau added inline comments.
================
Comment at: clang/lib/AST/TypePrinter.cpp:326
+ if (Policy.FullyQualifiedName && Policy.GlobalScopeQualifiedName &&
+ T->isStructureOrClassType()) {
+ OS << "::";
----------------
jblespiau wrote:
> sammccall wrote:
> > structure-or-class-type isn't quite the right check:
> > - enums and typedefs for instance should also be qualified.
> > - you're looking through sugar to the underlying type, which may not be
> > what you want
> >
> > It's going to be hard to get this right from here... I suspect it's better
> > to fix where `FullyQualifiedName` is checked in DeclPrinter.
> > This ultimately calls through to NamedDecl::printNestedNameSpecifier, which
> > is probably the right place to respect this option.
> > (I don't totally understand what's meant to happen if SuppressScope is
> > false but FullyQualiifedName is also false, that calls through to
> > NestedNameSpecifier::print which you may want to also fix, or not)
> There are several elements I do not understand (as you will see, I am
> completely lost).
>
> 1. I am trying to modify QualType::getAsString, and you suggest changing
> NamecDecl. I do not understand the relationship between Qualtype and
> NameDecl: I understand declaration within the context of C++:
> https://www.dummies.com/programming/cpp/expressions-and-declarations-in-c-programming/
> Thus, a Declaration will be made of several parts, some of which will be
> Type. Thus, for me, the change should not be in NameDecl but in Type, to also
> make sure the change is there both when we print a NameDecl and a type.. If
> we modify NameDecl, then, when printing a type through Type::getAsString, we
> won't get the global "::" specifier.
> I have understood NamedDecl::printQualifiedName calls
> NamedDecl::printNestedNameSpecifier which calls the Type::print function, see
> https://github.com/llvm-mirror/clang/blob/master/lib/AST/Decl.cpp#L1630
>
> => From that, I still understand I should modify how types are printed, not
> NamedDecl. I may completely be incorrect, and I am only looking to understand.
>
> Thus, I feel my change is correcly placed, but we can change it to be:
>
> if (!Policy.SuppressScope && Policy.GlobalScopeQualifiedName &&
> (T->isStructureOrClassType() || T->isEnumeralType() ||
> isa<TypedefType>(T))) {
> OS << "::";
> }
>
> Other remarks:
> 2. As documented:
> ```
> /// When true, print the fully qualified name of function declarations.
> /// This is the opposite of SuppressScope and thus overrules it.
> unsigned FullyQualifiedName : 1;
> ```
>
> FullyQualifiedName is only controlling the fully-qualification of functions.
> https://github.com/llvm/llvm-project/blob/a2a3e9f0a6e91103a0d1fa73086dbdf109c48f69/clang/lib/AST/DeclPrinter.cpp#L625
>
> So I do not think we have to follow this.
>
> I think it is `SuppressScope` which controls whether it is fully qualified or
> not,:
> https://github.com/llvm/llvm-project/blob/a2a3e9f0a6e91103a0d1fa73086dbdf109c48f69/clang/lib/AST/DeclPrinter.cpp#L629
>
> If Policy.SuppressScope is False, then I understand it's fully qualified.
>
> 3. "you're looking through sugar to the underlying type, which may not be
> what you want": I do not understand "sugar" in that context. I have read this
> name in the code in clang, but still, I do not understand it. I only know
> https://en.wikipedia.org/wiki/Syntactic_sugar, which does not seem to apply
> here (Obviously, I am not a native English speaker).
>
> For the testing:
>
> Building and running
> ------------------------
>
> After > 90minutes of building with:
> cmake -DLLVM_ENABLE_PROJECTS=clang -G "Unix Makefiles" ../llvm
> make check-clang
>
> It took me quite a while to find how to execute a single test file:
>
> ~/llvm-project/build/tools/clang/unittests/AST]
> └──╼ make -j12 ASTTests && ./ASTTests --gtest_filter=NamedDeclPrinter*
>
> did the job.
>
> - NamedDeclPrinterTest.cpp feels strange for the tests, as what I am
> modifying is not NamedDecl, but Qualtype::getAsString. But given there is no
> TypeTest.cpp, maybe it's best location.
>
>
> I must admit that I am struggling a lot to understand both the execution flow
> and the code itself :(
>
(I had issues with the formatting in Markdown. The big bold was not intended to
be big and bold^^ Sorry).
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80800/new/
https://reviews.llvm.org/D80800
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
