[PATCH] D80800: Add an option to fully qualify classes and structs.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D80800#2065643 , @jblespiau wrote:

> I did spend a few hours, just building and finding how to run tests :(


Sorry about that :-(
For what it's worth, building with Ninja should be significantly faster by 
default: https://llvm.org/docs/GettingStarted.html
But yes, building LLVM is very slow unless you have a powerful machine.

> 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.

Yes, I think you're right, sorry - I assumed these were sharing code. (This is 
why we need tests!)
Both cases need to be modified: PrintingPolicy is used for printing both Types 
and Decls, and the new option would need to apply to both cases.

The idea that the namespace prefix should be "sunk" down to where we know more 
about the structure of the type seems valid though: if you look at where the 
site you've found prints the scope, it calls printRecordBefore -> printTag -> 
AppendScope, the latter is probably the right place to modify for Type.




Comment at: clang/lib/AST/TypePrinter.cpp:326
+  if (Policy.FullyQualifiedName && Policy.GlobalScopeQualifiedName &&
+  T->isStructureOrClassType()) {
+OS << "::";

jblespiau wrote:
> 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(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
> > 
> > 

[PATCH] D80800: Add an option to fully qualify classes and structs.

[Jean-Baptiste Lespiau via Phabricator via cfe-commits]

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(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

[PATCH] D80800: Add an option to fully qualify classes and structs.

[Jean-Baptiste Lespiau via Phabricator via cfe-commits]

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(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

[PATCH] D80800: Add an option to fully qualify classes and structs.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This should really have a test - NamedDeclPrinterTest.cpp seems like the right 
place.




Comment at: clang/lib/AST/TypePrinter.cpp:326
+  if (Policy.FullyQualifiedName && Policy.GlobalScopeQualifiedName &&
+  T->isStructureOrClassType()) {
+OS << "::";

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)


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

[PATCH] D80800: Add an option to fully qualify classes and structs.

[Jean-Baptiste Lespiau via Phabricator via cfe-commits]

jblespiau created this revision.
jblespiau added a reviewer: sammccall.
jblespiau edited the summary of this revision.
sammccall added a comment.

This should really have a test - NamedDeclPrinterTest.cpp seems like the right 
place.




Comment at: clang/lib/AST/TypePrinter.cpp:326
+  if (Policy.FullyQualifiedName && Policy.GlobalScopeQualifiedName &&
+  T->isStructureOrClassType()) {
+OS << "::";

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)


Generally speaking, using TypePrinter is the most flexible mechanism for
printing types. It makes sense to have this feature just for this
reason.

However, this is also done in another context:
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20200518/321225.html

In a nutshell, `getFullyQualifiedName` is bugged and prints
`::tensorfn::Nested< ::std::variant >`
instead of:
`::tensorfn::Nested< 
::std::variant<::tensorflow::Tensor,::tensorfn::DeviceTensor> >`

I was not able to fix this function, and intend to add this feature and
then start to remove the use of the other function (in CLIF first).

Longer term, we should delete `QualTypeNames.cpp`, and prefer
TypePrinter.

Things that are not that clear:

- should more types than classes and struct be globally qualified?


https://reviews.llvm.org/D80800

Files:
  clang/include/clang/AST/PrettyPrinter.h
  clang/lib/AST/TypePrinter.cpp


Index: clang/lib/AST/TypePrinter.cpp
===
--- clang/lib/AST/TypePrinter.cpp
+++ clang/lib/AST/TypePrinter.cpp
@@ -19,6 +19,7 @@
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/NestedNameSpecifier.h"
+#include "clang/AST/PrettyPrinter.h"
 #include "clang/AST/TemplateBase.h"
 #include "clang/AST/TemplateName.h"
 #include "clang/AST/Type.h"
@@ -40,6 +41,7 @@
 #include "llvm/Support/SaveAndRestore.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
+#include 
 #include 
 
 using namespace clang;
@@ -320,6 +322,10 @@
   HasEmptyPlaceHolder = false;
   }
 
+  if (Policy.FullyQualifiedName && Policy.GlobalScopeQualifiedName &&
+  T->isStructureOrClassType()) {
+OS << "::";
+  }
   switch (T->getTypeClass()) {
 #define ABSTRACT_TYPE(CLASS, PARENT)
 #define TYPE(CLASS, PARENT) case Type::CLASS: \
Index: clang/include/clang/AST/PrettyPrinter.h
===
--- clang/include/clang/AST/PrettyPrinter.h
+++ clang/include/clang/AST/PrettyPrinter.h
@@ -63,7 +63,8 @@
 MSWChar(LO.MicrosoftExt && !LO.WChar), IncludeNewlines(true),
 MSVCFormatting(false), ConstantsAsWritten(false),
 SuppressImplicitBase(false), FullyQualifiedName(false),
-PrintCanonicalTypes(false), PrintInjectedClassNameWithArguments(true) 
{}
+GlobalScopeQualifiedName(false), PrintCanonicalTypes(false),
+PrintInjectedClassNameWithArguments(true) {}
 
   /// Adjust this printing policy for cases where it's known that we're
   /// printing C++ code (for instance, if AST dumping reaches a C++-only
@@ -241,6 +242,13 @@
   /// This is the opposite of SuppressScope and thus overrules it.
   unsigned FullyQualifiedName : 1;
 
+  // When true, class and struct types (to be expanded if needed) will be
+  // prepended with "::"
+  // Note it also requires `FullyQualifiedName` to also be set to true, as it
+  // does not make sense to prepend "::" to a non fully-qualified name.
+  // This targets generated code.
+  unsigned GlobalScopeQualifiedName : 1;
+
   /// Whether to print types as written or canonically.
   unsigned PrintCanonicalTypes : 1;
 


Index: clang/lib/AST/TypePrinter.cpp
===
--- clang/lib/AST/TypePrinter.cpp
+++ clang/lib/AST/TypePrinter.cpp
@@ -19,6 +19,7 @@
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/NestedNameSpecifier.h"
+#include "clang/AST/PrettyPrinter.h"
 #include "clang/AST/TemplateBase.h"
 #include "clang/AST/TemplateName.h"
 #include "clang/AST/Type.h"
@@ -40,6 +41,7 @@
 #include "llvm/Support/SaveAndRestore.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
+#include 
 #include 
 
 using namespace clang;
@@ -320,6 +322,10 @@
   HasEmptyPlaceHolder = false;
   }
 
+  if (Policy.FullyQualifiedName &&