[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<Tensor, DeviceTensor> >`
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 <cassert>
+#include <iostream>
 #include <string>
 
 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;
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D80800.267224.patch
Type: text/x-patch
Size: 2267 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200529/8870bc41/attachment-0001.bin>