[clang-cl] Fix value of FUNCTION in MSVC mode. #84014

zahiraam · 2024-03-05T13:37:10Z

Predefined macro FUNCTION in clang is not returning the same string than MS for templated functions.

For the same test case MSVC is returning:

function: TestClass::TestClass
function: TestStruct::TestStruct
function: TestEnum::TestEnum

The initial work for this was in the reverted patch (#66120). This patch solves the issues raised in the reverted patch.

clang/lib/AST/Expr.cpp

clang/lib/Sema/SemaExpr.cpp

clang/test/Analysis/eval-predefined-exprs.cpp

clang/test/SemaCXX/source_location.cpp

clang/lib/AST/TypePrinter.cpp

Sirraide · 2024-03-05T16:07:15Z

I’m not entirely sure this change is a good idea: some people might be relying on Clang’s behaviour wrt __FUNCTION__, especially with how long it’s been around; Clang may not be fully MSVC-compatible in this case, but that incompatibilty seems unlikely to cause problems imo since afaik most people that actually depend on the output of these sorts of macros for e.g. reflexion use __FUNCSIG__ or __PRETTY_FUNCTION__ instead—then again, that could also be a reason as to why changing it might actually be fine.

zahiraam · 2024-03-05T16:12:38Z

I’m not entirely sure this change is a good idea: some people might be relying on Clang’s behaviour wrt __FUNCTION__, especially with how long it’s been around; Clang may not be fully MSVC-compatible in this case, but that incompatibilty seems unlikely to cause problems imo since afaik most people that actually depend on the output of these sorts of macros for e.g. reflexion use __FUNCSIG__ or __PRETTY_FUNCTION__ instead—then again, that could also be a reason as to why changing it might actually be fine.

Thanks for the review. This is an issue that we encountered in our internal testing with an application using __FUNCTION__. I agree that might not be used by a lot of users/apps, but it is used and having it compatible with MSVC doesn't actually hurt? (I hope).

llvmbot · 2024-03-05T16:13:23Z

@llvm/pr-subscribers-clang

Author: Zahira Ammarguellat (zahiraam)

Changes

Predefined macro FUNCTION in clang is not returning the same string than MS for templated functions.

See https://godbolt.org/z/q3EKn5zq4

For the same test case MSVC is returning:

function: TestClass::TestClass
function: TestStruct::TestStruct
function: TestEnum::TestEnum

Full diff: https://github.com/llvm/llvm-project/pull/84014.diff

9 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+3)
(modified) clang/include/clang/AST/Expr.h (+2-1)
(modified) clang/lib/AST/Expr.cpp (+21-5)
(modified) clang/lib/AST/TypePrinter.cpp (+20-4)
(modified) clang/lib/Sema/SemaExpr.cpp (+4-1)
(modified) clang/test/AST/Interp/literals.cpp (+4-4)
(modified) clang/test/Analysis/eval-predefined-exprs.cpp (+16-6)
(modified) clang/test/SemaCXX/source_location.cpp (+64)
(modified) clang/unittests/AST/DeclPrinterTest.cpp (+15)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 612b4329727455..20c14fae1dd31b 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -224,6 +224,9 @@ Bug Fixes in This Version
   for variables created through copy initialization having side-effects in C++17 and later.
   Fixes (#GH64356) (#GH79518).
 
+- Fix value of predefined macro ``__FUNCTION__`` to match MSVC's value. Fixes
+  (`#66114 <https://github.com/llvm/llvm-project/issues/66114>`_).
+
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
diff --git a/clang/include/clang/AST/Expr.h b/clang/include/clang/AST/Expr.h
index bf0622bdeca30e..ce8e64a4bed04b 100644
--- a/clang/include/clang/AST/Expr.h
+++ b/clang/include/clang/AST/Expr.h
@@ -2034,7 +2034,8 @@ class PredefinedExpr final
   }
 
   static std::string ComputeName(PredefinedIdentKind IK,
-                                 const Decl *CurrentDecl);
+                                 const Decl *CurrentDecl,
+                                 bool ForceElaboratedPrinting = false);
 
   SourceLocation getBeginLoc() const { return getLocation(); }
   SourceLocation getEndLoc() const { return getLocation(); }
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index b4de2155adcebd..796e50817ee319 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -673,7 +673,8 @@ StringRef PredefinedExpr::getIdentKindName(PredefinedIdentKind IK) {
 // FIXME: Maybe this should use DeclPrinter with a special "print predefined
 // expr" policy instead.
 std::string PredefinedExpr::ComputeName(PredefinedIdentKind IK,
-                                        const Decl *CurrentDecl) {
+                                        const Decl *CurrentDecl,
+                                        bool ForceElaboratedPrinting) {
   ASTContext &Context = CurrentDecl->getASTContext();
 
   if (IK == PredefinedIdentKind::FuncDName) {
@@ -721,10 +722,17 @@ std::string PredefinedExpr::ComputeName(PredefinedIdentKind IK,
     return std::string(Out.str());
   }
   if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(CurrentDecl)) {
-    if (IK != PredefinedIdentKind::PrettyFunction &&
-        IK != PredefinedIdentKind::PrettyFunctionNoVirtual &&
-        IK != PredefinedIdentKind::FuncSig &&
-        IK != PredefinedIdentKind::LFuncSig)
+    const auto &LO = Context.getLangOpts();
+    if ((ForceElaboratedPrinting &&
+         (((IK == PredefinedIdentKind::Func ||
+            IK == PredefinedIdentKind ::Function) &&
+           !LO.MicrosoftExt) ||
+          (IK == PredefinedIdentKind::LFunction && LO.MicrosoftExt))) ||
+        (!ForceElaboratedPrinting &&
+         (IK != PredefinedIdentKind::PrettyFunction &&
+          IK != PredefinedIdentKind::PrettyFunctionNoVirtual &&
+          IK != PredefinedIdentKind::FuncSig &&
+          IK != PredefinedIdentKind::LFuncSig)))
       return FD->getNameAsString();
 
     SmallString<256> Name;
@@ -752,6 +760,8 @@ std::string PredefinedExpr::ComputeName(PredefinedIdentKind IK,
     PrintingPolicy Policy(Context.getLangOpts());
     PrettyCallbacks PrettyCB(Context.getLangOpts());
     Policy.Callbacks = &PrettyCB;
+    if (IK == PredefinedIdentKind::Function && ForceElaboratedPrinting)
+      Policy.SuppressTagKeyword = !LO.MicrosoftExt;
     std::string Proto;
     llvm::raw_string_ostream POut(Proto);
 
@@ -779,6 +789,12 @@ std::string PredefinedExpr::ComputeName(PredefinedIdentKind IK,
 
     FD->printQualifiedName(POut, Policy);
 
+    if (IK == PredefinedIdentKind::Function) {
+      POut.flush();
+      Out << Proto;
+      return std::string(Name);
+    }
+
     POut << "(";
     if (FT) {
       for (unsigned i = 0, e = Decl->getNumParams(); i != e; ++i) {
diff --git a/clang/lib/AST/TypePrinter.cpp b/clang/lib/AST/TypePrinter.cpp
index 7dcc4348f8e036..21605e1f53e3d9 100644
--- a/clang/lib/AST/TypePrinter.cpp
+++ b/clang/lib/AST/TypePrinter.cpp
@@ -1635,6 +1635,17 @@ void TypePrinter::printElaboratedBefore(const ElaboratedType *T,
     if (T->getKeyword() != ElaboratedTypeKeyword::None)
       OS << " ";
     NestedNameSpecifier *Qualifier = T->getQualifier();
+    if (!Policy.SuppressTagKeyword && Policy.SuppressScope &&
+        !Policy.SuppressUnwrittenScope) {
+      std::string prefix = T->isClassType()       ? "class "
+                           : T->isStructureType() ? "struct "
+                           : T->isUnionType()     ? "union "
+                                                  : "";
+      OS << prefix;
+      Policy.SuppressTagKeyword = true;
+      Policy.SuppressScope = false;
+      return printBefore(T->getNamedType(), OS);
+    }
     if (Qualifier)
       Qualifier->print(OS, Policy);
   }
@@ -2260,10 +2271,15 @@ printTo(raw_ostream &OS, ArrayRef<TA> Args, const PrintingPolicy &Policy,
     } else {
       if (!FirstArg)
         OS << Comma;
-      // Tries to print the argument with location info if exists.
-      printArgument(Arg, Policy, ArgOS,
-                    TemplateParameterList::shouldIncludeTypeForArgument(
-                        Policy, TPL, ParmIndex));
+      if (!Policy.SuppressTagKeyword &&
+          Argument.getKind() == TemplateArgument::Type &&
+          isa<TagType>(Argument.getAsType()))
+        OS << Argument.getAsType().getAsString().data();
+      else
+        // Tries to print the argument with location info if exists.
+        printArgument(Arg, Policy, ArgOS,
+                      TemplateParameterList::shouldIncludeTypeForArgument(
+                          Policy, TPL, ParmIndex));
     }
     StringRef ArgString = ArgOS.str();
 
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 0a449fc1082bd4..fa0daa8ab0491b 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -3741,7 +3741,10 @@ ExprResult Sema::BuildPredefinedExpr(SourceLocation Loc,
   else {
     // Pre-defined identifiers are of type char[x], where x is the length of
     // the string.
-    auto Str = PredefinedExpr::ComputeName(IK, currentDecl);
+    bool ForceElaboratedPrinting =
+        IK == PredefinedIdentKind::Function && getLangOpts().MicrosoftExt;
+    auto Str =
+        PredefinedExpr::ComputeName(IK, currentDecl, ForceElaboratedPrinting);
     unsigned Length = Str.length();
 
     llvm::APInt LengthI(32, Length + 1);
diff --git a/clang/test/AST/Interp/literals.cpp b/clang/test/AST/Interp/literals.cpp
index d86609108ca446..b2f3f2cf7e336f 100644
--- a/clang/test/AST/Interp/literals.cpp
+++ b/clang/test/AST/Interp/literals.cpp
@@ -1039,7 +1039,7 @@ namespace PredefinedExprs {
     static_assert(strings_match(__FUNCSIG__, "void __cdecl PredefinedExprs::foo(void)"), "");
     static_assert(strings_match(L__FUNCSIG__, L"void __cdecl PredefinedExprs::foo(void)"), "");
     static_assert(strings_match(L__FUNCTION__, L"foo"), "");
-    static_assert(strings_match(__FUNCTION__, "foo"), "");
+    static_assert(strings_match(__FUNCTION__, "PredefinedExprs::foo"), "");
     static_assert(strings_match(__func__, "foo"), "");
     static_assert(strings_match(__PRETTY_FUNCTION__, "void PredefinedExprs::foo()"), "");
   }
@@ -1049,9 +1049,9 @@ namespace PredefinedExprs {
     __extension__ __FUNCTION__; // both-warning {{result unused}}
     return __FUNCTION__[index];
   }
-  static_assert(heh(0) == 'h', "");
-  static_assert(heh(1) == 'e', "");
-  static_assert(heh(2) == 'h', "");
+  static_assert(heh(0) == 'P', "");
+  static_assert(heh(1) == 'r', "");
+  static_assert(heh(2) == 'e', "");
 #endif
 }
 
diff --git a/clang/test/Analysis/eval-predefined-exprs.cpp b/clang/test/Analysis/eval-predefined-exprs.cpp
index 1eec4476a065f3..a6bac5ee9d486d 100644
--- a/clang/test/Analysis/eval-predefined-exprs.cpp
+++ b/clang/test/Analysis/eval-predefined-exprs.cpp
@@ -55,9 +55,14 @@ struct A {
     clang_analyzer_dump(__func__);
     clang_analyzer_dump(__FUNCTION__);
     clang_analyzer_dump(__PRETTY_FUNCTION__);
-    // expected-warning@-3 {{&Element{"A",0 S64b,char}}}
-    // expected-warning@-3 {{&Element{"A",0 S64b,char}}}
-    // expected-warning@-3 {{&Element{"A::A()",0 S64b,char}}}
+#ifdef ANALYZER_MS
+    // expected-warning@-4 {{&Element{"A",0 S64b,char}}}
+    // expected-warning@-4 {{&Element{"A::A",0 S64b,char}}}
+#else
+    // expected-warning@-7 {{&Element{"A",0 S64b,char}}}
+    // expected-warning@-7 {{&Element{"A",0 S64b,char}}}
+#endif
+    // expected-warning@-8 {{&Element{"A::A()",0 S64b,char}}}
 
 #ifdef ANALYZER_MS
     clang_analyzer_dump(__FUNCDNAME__);
@@ -74,9 +79,14 @@ struct A {
     clang_analyzer_dump(__func__);
     clang_analyzer_dump(__FUNCTION__);
     clang_analyzer_dump(__PRETTY_FUNCTION__);
-    // expected-warning@-3 {{&Element{"~A",0 S64b,char}}}
-    // expected-warning@-3 {{&Element{"~A",0 S64b,char}}}
-    // expected-warning@-3 {{&Element{"A::~A()",0 S64b,char}}}
+#ifdef ANALYZER_MS
+    // expected-warning@-4 {{&Element{"~A",0 S64b,char}}}
+    // expected-warning@-4 {{&Element{"A::~A",0 S64b,char}}}
+#else
+    // expected-warning@-7 {{&Element{"~A",0 S64b,char}}}
+    // expected-warning@-7 {{&Element{"~A",0 S64b,char}}}
+#endif
+    // expected-warning@-8 {{&Element{"A::~A()",0 S64b,char}}}
 
 #ifdef ANALYZER_MS
     clang_analyzer_dump(__FUNCDNAME__);
diff --git a/clang/test/SemaCXX/source_location.cpp b/clang/test/SemaCXX/source_location.cpp
index 7414fbce7828d1..203925cf49e936 100644
--- a/clang/test/SemaCXX/source_location.cpp
+++ b/clang/test/SemaCXX/source_location.cpp
@@ -463,8 +463,72 @@ void ctor_tests() {
 constexpr SL global_sl = SL::current();
 static_assert(is_equal(global_sl.function(), ""));
 
+template <class T>
+class TestBI {
+public:
+   TestBI() {
+#ifdef MS
+     static_assert(is_equal(__FUNCTION__, "test_func::TestBI<int>::TestBI"));
+     static_assert(is_equal(__func__, "TestBI"));
+#else
+     static_assert(is_equal(__FUNCTION__, "TestBI"));
+     static_assert(is_equal(__func__, "TestBI"));
+#endif
+   }
+};
+
+template <class T>
+class TestClass {
+public:
+   TestClass() {
+#ifdef MS
+      static_assert(is_equal(__FUNCTION__, "test_func::TestClass<class test_func::C>::TestClass"));
+      static_assert(is_equal(__func__, "TestClass"));
+#else
+      static_assert(is_equal(__FUNCTION__, "TestClass"));
+      static_assert(is_equal(__func__, "TestClass"));
+#endif
+   }
+};
+
+template <class T>
+class TestStruct {
+public:
+   TestStruct() {
+#ifdef MS
+      static_assert(is_equal(__FUNCTION__, "test_func::TestStruct<struct test_func::S>::TestStruct"));
+      static_assert(is_equal(__func__, "TestStruct"));
+#else
+      static_assert(is_equal(__FUNCTION__, "TestStruct"));
+      static_assert(is_equal(__func__, "TestStruct"));
+#endif
+   }
+};
+
+template <class T>
+class TestEnum {
+public:
+   TestEnum() {
+#ifdef MS
+      static_assert(is_equal(__FUNCTION__, "test_func::TestEnum<enum test_func::E>::TestEnum"));
+      static_assert(is_equal(__func__, "TestEnum"));
+#else
+      static_assert(is_equal(__FUNCTION__, "TestEnum"));
+      static_assert(is_equal(__func__, "TestEnum"));
+#endif
+   }
+};
+
+class C {};
+struct S {};
+enum E {};
 } // namespace test_func
 
+test_func::TestBI<int> t1;
+test_func::TestClass<test_func::C> t2;
+test_func::TestStruct<test_func::S> t3;
+test_func::TestEnum<test_func::E> t4;
+
 //===----------------------------------------------------------------------===//
 //                            __builtin_FUNCSIG()
 //===----------------------------------------------------------------------===//
diff --git a/clang/unittests/AST/DeclPrinterTest.cpp b/clang/unittests/AST/DeclPrinterTest.cpp
index 0e09ab2a7bba88..8fcfaa7c2f0379 100644
--- a/clang/unittests/AST/DeclPrinterTest.cpp
+++ b/clang/unittests/AST/DeclPrinterTest.cpp
@@ -358,6 +358,21 @@ TEST(DeclPrinter, TestCXXRecordDecl11) {
     "class A : virtual public Z, private Y {}"));
 }
 
+TEST(DeclPrinter, TestCXXRecordDecl12) {
+  ASSERT_TRUE(PrintedDeclCXX98Matches(
+      "struct S { int x; };"
+      "namespace NS { class C {};}"
+      "S foo(S s1, NS::C c1) {using namespace NS; C c; return s1;}",
+      "foo",
+      "struct S foo(struct S s1, class NS::C c1) {\nusing namespace NS;\nclass "
+      "NS::C c;\nreturn s1;\n}\n",
+      [](PrintingPolicy &Policy) {
+        Policy.SuppressTagKeyword = false;
+        Policy.SuppressScope = true;
+        Policy.TerseOutput = false;
+      }));
+}
+
 TEST(DeclPrinter, TestFunctionDecl1) {
   ASSERT_TRUE(PrintedDeclCXX98Matches(
     "void A();",

Sirraide · 2024-03-05T16:26:13Z

Oh, I just noticed that you’ve tried to land this change before (#66120), but it seems it was reverted because of some problems; if those are fixed now, then I’d say going forward with this is probably fine, seeing as it was approved the first time round.

This would have been a bit less confusing if you had pointed that out, just saying ;Þ

zahiraam · 2024-03-05T16:28:36Z

Oh, I just noticed that you’ve tried to land this change before (#66120), but it seems it was reverted because of some problems; if those are fixed now, then I’d say going forward with this is probably fine, seeing as it was approved the first time round.

This would have been a bit less confusing if you had pointed that out, just saying ;Þ

Yes. Sorry about that. This patch was mostly to fix the issues raised in PR#66120.

clang/test/Analysis/eval-predefined-exprs.cpp

clang/docs/ReleaseNotes.rst

Sirraide · 2024-03-06T12:55:10Z

Alright, the changes look sensible to me, but I’d like for someone who’s more familiar with this part of Clang or with the original pr to comment on them too before we merge it.

cor3ntin · 2024-03-06T13:08:44Z

clang/docs/ReleaseNotes.rst

@@ -224,6 +224,9 @@ Bug Fixes in This Version
  for variables created through copy initialization having side-effects in C++17 and later.
  Fixes (#GH64356) (#GH79518).

+- Fix value of predefined macro ``__FUNCTION__`` in MSVC compatibility mode.
+  Fixes (GH#66114).


Suggested change

Fixes (GH#66114).

Fixes (#GH66114).

AaronBallman · 2024-03-06T13:17:13Z

clang/lib/AST/Expr.cpp

+    bool isFuncOrFunctionInNonMSVCCompatEnv =
+        ((IK == PredefinedIdentKind::Func ||
+          IK == PredefinedIdentKind ::Function) &&
+         !LO.MSVCCompat);
+    bool isLFunctionInMSVCCommpatEnv =
+        IK == PredefinedIdentKind::LFunction && LO.MSVCCompat;
+    bool isFuncOrFunctionOrLFunctionOrFuncDName =


Suggested change

bool isFuncOrFunctionInNonMSVCCompatEnv =

((IK == PredefinedIdentKind::Func ||

IK == PredefinedIdentKind ::Function) &&

!LO.MSVCCompat);

bool isLFunctionInMSVCCommpatEnv =

IK == PredefinedIdentKind::LFunction && LO.MSVCCompat;

bool isFuncOrFunctionOrLFunctionOrFuncDName =

bool IsFuncOrFunctionInNonMSVCCompatEnv =

((IK == PredefinedIdentKind::Func ||

IK == PredefinedIdentKind ::Function) &&

!LO.MSVCCompat);

bool IsLFunctionInMSVCCommpatEnv =

IK == PredefinedIdentKind::LFunction && LO.MSVCCompat;

bool IsFuncOrFunctionOrLFunctionOrFuncDName =

Coding style nits, NFC

AaronBallman · 2024-03-06T13:19:56Z

clang/lib/AST/Expr.cpp

+        IK != PredefinedIdentKind::LFuncSig;
+    if ((ForceElaboratedPrinting &&
+         (isFuncOrFunctionInNonMSVCCompatEnv || isLFunctionInMSVCCommpatEnv)) ||
+        !ForceElaboratedPrinting && isFuncOrFunctionOrLFunctionOrFuncDName)


Suggested change

!ForceElaboratedPrinting && isFuncOrFunctionOrLFunctionOrFuncDName)

(!ForceElaboratedPrinting && isFuncOrFunctionOrLFunctionOrFuncDName))

You may need this to avoid diagnostics about mixed && and || operators.

AaronBallman · 2024-03-06T13:21:36Z

clang/lib/AST/TypePrinter.cpp

+      Policy.SuppressTagKeyword = true;
+      Policy.SuppressScope = false;


Don't we want to reset these once we're done with the call to printBefore()?

Yes, I think so.

Hmmm, your changes don't look quite right to me. The return printBefore(...); is going to skip right over the restoration.

I think you wanted something more like:

bool OldTag = Policy.SuppressTagKeyword, OldScope = Policy.SuppressScope; Policy.SuppressTagKeyword = true; Policy.SuppressScope = false; printBefore(T->getNamedType(), OS); Policy.SuppressTagKeyword = OldTag; Policy.SuppressScope = OldScope; return;

Before making the changes, I think you should also add test coverage to the unit test where these changes will fix the test.

Not sure what you mean here "where these changes will fix the test"? will fix which test? The unit test you are referring to is DeclPrinterTest? this should be TestCXXRecordDecl12.

The code was broken but no tests failed, then the fix you made was also broken but still no tests failed. So I'm asking you to write a new test that demonstrates incorrect behavior related to failing to reset the printing policy after modifying it, then fix the code until that test shows the correct behavior.

Hmmm! Can't seem to find a test case that fails. At each step of the printing ElaboratedTypePolicyRAII PolicyRAII(Policy); gets called and that re-initializes the values of Policy.SuppressTagKeyword and Policy.SuppressScope to true!

You need to reset the values here, or use a separate PrintingPolicy object so that you're not changing the value for the default printing policy used.

github-actions · 2024-03-14T13:18:42Z

✅ With the latest revision this PR passed the C/C++ code formatter.

Sirraide · 2024-03-18T11:14:51Z

From what I can tell, it seems that Aaron is a lot more familiar w/ this part of Clang than me, so I’ll leave it to him to comment on whether these changes make sense.

AaronBallman · 2024-03-18T13:54:42Z

clang/lib/AST/DeclPrinter.cpp

@@ -679,6 +679,19 @@ static void printExplicitSpecifier(ExplicitSpecifier ES, llvm::raw_ostream &Out,
  Out << Proto;
 }

+static void AddPrefix(PrintingPolicy &Policy, QualType T,


This is a pretty non-descriptive function name; the caller will have no idea what kind of prefix it adds. But trying to come up with a name for this is... not easy.

MaybePrintTagKeywordIfSupressingScopes is not super great either. :-D

AaronBallman · 2024-03-18T13:55:25Z

clang/lib/AST/DeclPrinter.cpp

+    StringRef prefix;
+    prefix = T->isClassType()       ? "class "
+             : T->isStructureType() ? "struct "
+             : T->isUnionType()     ? "union "
+                                    : "";
+    Out << prefix;


Suggested change

StringRef prefix;

prefix = T->isClassType() ? "class "

: T->isStructureType() ? "struct "

: T->isUnionType() ? "union "

: "";

Out << prefix;

StringRef prefix = T->isClassType() ? "class "

: T->isStructureType() ? "struct "

: T->isUnionType() ? "union "

: "";

Out << prefix;

And probably needs to be reformatted.

AaronBallman · 2024-03-18T14:22:00Z

clang/lib/AST/DeclPrinter.cpp

+      if (!Policy.SuppressTagKeyword && Policy.SuppressScope &&
+          !Policy.SuppressUnwrittenScope) {
+        AddPrefix(Policy, AFT->getReturnType(), Out);


This check is duplicated in AddPrefix(), same below (probably can remove the check from AddPrefix() or turn it into an assertion).

Removed it from MaybePrintTagKeywordIfSupressingScopes .

AaronBallman · 2024-03-18T14:35:02Z

clang/lib/AST/DeclPrinter.cpp

+        bool OldTagKeyword = Policy.SuppressTagKeyword;
+        bool OldSupressScope = Policy.SuppressScope;
+        AFT->getReturnType().print(Out, Policy, Proto);
+        Policy.SuppressTagKeyword = OldTagKeyword;
+        Policy.SuppressScope = OldSupressScope;


This is only necessary because of the changes you made in TypePrinter.cpp, right? e.g., if we reset the policy back to its original values there, we don't need to do it here.

AaronBallman · 2024-03-18T14:35:09Z

clang/lib/AST/DeclPrinter.cpp

+    bool OldTagKeyword = Policy.SuppressTagKeyword;
+    bool OldSupressScope = Policy.SuppressScope;
+    printDeclType(T, Name);
+    Policy.SuppressTagKeyword = OldTagKeyword;
+    Policy.SuppressScope = OldSupressScope;


AaronBallman · 2024-03-18T14:37:12Z

clang/lib/AST/TypePrinter.cpp

+      Policy.SuppressTagKeyword = true;
+      Policy.SuppressScope = false;


You need to reset the values here, or use a separate PrintingPolicy object so that you're not changing the value for the default printing policy used.

AaronBallman

I spotted some simplifications that can be applied. If precommit CI comes back green, then I think this is basically ready to go.

AaronBallman · 2024-03-18T18:45:49Z

clang/lib/AST/DeclPrinter.cpp

+      if (!Policy.SuppressTagKeyword && Policy.SuppressScope &&
+          !Policy.SuppressUnwrittenScope) {
+        MaybePrintTagKeywordIfSupressingScopes(Policy, AFT->getReturnType(),
+                                               Out);
+        AFT->getReturnType().print(Out, Policy, Proto);
+      } else {
+        AFT->getReturnType().print(Out, Policy, Proto);
+      }


Suggested change

if (!Policy.SuppressTagKeyword && Policy.SuppressScope &&

!Policy.SuppressUnwrittenScope) {

MaybePrintTagKeywordIfSupressingScopes(Policy, AFT->getReturnType(),

Out);

AFT->getReturnType().print(Out, Policy, Proto);

} else {

AFT->getReturnType().print(Out, Policy, Proto);

}

if (!Policy.SuppressTagKeyword && Policy.SuppressScope &&

!Policy.SuppressUnwrittenScope)

MaybePrintTagKeywordIfSupressingScopes(Policy, AFT->getReturnType(),

Out);

AFT->getReturnType().print(Out, Policy, Proto);

AaronBallman · 2024-03-18T18:46:25Z

clang/lib/AST/DeclPrinter.cpp

+  if (!Policy.SuppressTagKeyword && Policy.SuppressScope &&
+      !Policy.SuppressUnwrittenScope) {
+    MaybePrintTagKeywordIfSupressingScopes(Policy, T, Out);
+    printDeclType(T, Name);
+  } else {
+    printDeclType(T, Name);
+  }


Suggested change

if (!Policy.SuppressTagKeyword && Policy.SuppressScope &&

!Policy.SuppressUnwrittenScope) {

MaybePrintTagKeywordIfSupressingScopes(Policy, T, Out);

printDeclType(T, Name);

} else {

printDeclType(T, Name);

}

if (!Policy.SuppressTagKeyword && Policy.SuppressScope &&

!Policy.SuppressUnwrittenScope)

MaybePrintTagKeywordIfSupressingScopes(Policy, T, Out);

printDeclType(T, Name);

zahiraam · 2024-03-19T11:11:07Z

Thank you all for the reviews.

Predefined macro FUNCTION in clang is not returning the same string than MS for templated functions. See https://godbolt.org/z/q3EKn5zq4 For the same test case MSVC is returning: function: TestClass::TestClass function: TestStruct::TestStruct function: TestEnum::TestEnum The initial work for this was in the reverted patch (llvm#66120). This patch solves the issues raised in the reverted patch.

An upcoming LLVM change (llvm/llvm-project#84014) will make it so that the __FUNCTION__ macro will expand to the fully qulified name of class/struct methods instead of only the method name itself. That change is consistent with what MSVC does. To fix this, we fix BeforeStateCheckAction(...) so that it checks that the substring "Check" is present anywhere in the function name rather than specifically at the beginning of the function name. Bug: 331665490 Fixed: 331665490 Change-Id: Id7bce021d9b18b7db1805f37fbc08a6fc0bb3647 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5402860 Auto-Submit: Alan Zhao <[email protected]> Commit-Queue: Alan Zhao <[email protected]> Reviewed-by: Mike Wasserman <[email protected]> Cr-Commit-Position: refs/heads/main@{#1279841}

[clang-cl] Fix value of __FUNCTION__ in MSVC mode.

bdefe75

Sirraide requested changes Mar 5, 2024

View reviewed changes

zahiraam requested a review from AaronBallman March 5, 2024 16:07

Sirraide added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label Mar 5, 2024

zahiraam marked this pull request as ready for review March 5, 2024 16:12

llvmbot added the clang Clang issues not falling into any other category label Mar 5, 2024

Sirraide removed the clang Clang issues not falling into any other category label Mar 5, 2024

Addressed review comments.

13f4693

llvmbot added the clang Clang issues not falling into any other category label Mar 5, 2024

Sirraide requested changes Mar 5, 2024

View reviewed changes

clang/test/Analysis/eval-predefined-exprs.cpp Outdated Show resolved Hide resolved

clang/test/Analysis/eval-predefined-exprs.cpp Outdated Show resolved Hide resolved

clang/docs/ReleaseNotes.rst Outdated Show resolved Hide resolved

Addressed review comments.

5eeffd1

Sirraide approved these changes Mar 6, 2024

View reviewed changes

zahiraam requested review from rnk, cor3ntin and Caslyn March 6, 2024 12:57

cor3ntin reviewed Mar 6, 2024

View reviewed changes

Fixed typo.

16d3c75

AaronBallman reviewed Mar 6, 2024

View reviewed changes

zahiraam added 2 commits March 6, 2024 06:32

Addressed review comment.

463cbc9

Refactor the code and added entry to TypePrinterTest.cpp.

4b42f76

zahiraam added 2 commits March 14, 2024 07:34

Fix format.

878b179

Fix format again.

bdcb510

zahiraam requested a review from cor3ntin March 18, 2024 11:05

zahiraam requested review from Sirraide and AaronBallman March 18, 2024 11:05

AaronBallman reviewed Mar 18, 2024

View reviewed changes

Responded review comments.

efeb7b0

zahiraam requested a review from AaronBallman March 18, 2024 16:14

AaronBallman reviewed Mar 18, 2024

View reviewed changes

zahiraam added 2 commits March 18, 2024 12:09

Simplification applied.

9f16b25

Merge branch 'main' into ValueFuncMSVC

09b04af

zahiraam merged commit 6503b01 into llvm:main Mar 19, 2024

zahiraam deleted the ValueFuncMSVC branch March 19, 2024 15:00

AaronBallman added a commit that referenced this pull request Mar 19, 2024

Fixing up an edit missed in #84014; NFC

9ed1aa3

chencha3 pushed a commit to chencha3/llvm-project that referenced this pull request Mar 23, 2024

Fixing up an edit missed in llvm#84014; NFC

c94eded

zahiraam mentioned this pull request Mar 26, 2024

Fix printing of templated records. #86339

Merged

	!ForceElaboratedPrinting && isFuncOrFunctionOrLFunctionOrFuncDName)
	(!ForceElaboratedPrinting && isFuncOrFunctionOrLFunctionOrFuncDName))

		Policy.SuppressTagKeyword = true;
		Policy.SuppressScope = false;

[clang-cl] Fix value of __FUNCTION__ in MSVC mode. #84014

[clang-cl] Fix value of __FUNCTION__ in MSVC mode. #84014

Uh oh!

Conversation

zahiraam commented Mar 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Sirraide commented Mar 5, 2024

Uh oh!

zahiraam commented Mar 5, 2024

Uh oh!

llvmbot commented Mar 5, 2024

Uh oh!

Sirraide commented Mar 5, 2024

Uh oh!

zahiraam commented Mar 5, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Sirraide commented Mar 6, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sirraide commented Mar 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zahiraam commented Mar 19, 2024

Uh oh!

[clang-cl] Fix value of FUNCTION in MSVC mode. #84014

[clang-cl] Fix value of FUNCTION in MSVC mode. #84014

zahiraam commented Mar 5, 2024 •

edited

Loading

github-actions bot commented Mar 14, 2024 •

edited

Loading