jblespiau marked an inline comment as done.
jblespiau added a comment.
I did spend a few hours, just building and finding how to run tests :(
I have a few additional questions, as I do not really understand what happen.
In my initial idea, I wanted to modify the way QualType are printed, not really
declarations, but I feel the logic is duplicated somewhere, and that both
NamedDecl and Type are implementing the logic.
================
Comment at: clang/lib/AST/TypePrinter.cpp:326
+ if (Policy.FullyQualifiedName && Policy.GlobalScopeQualifiedName &&
+ T->isStructureOrClassType()) {
+ OS << "::";
----------------
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 :(
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
