-
Notifications
You must be signed in to change notification settings - Fork 11.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[clang-cl] Fix value of __FUNCTION__ in MSVC mode. #67592
base: main
Are you sure you want to change the base?
Conversation
Could you add a description explaining what is currently wrong with the value of |
Hope the RN is at the right place? Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd appreciate some unit tests showing that we print the elaboration in other circumstances. e.g.,
struct S { int x; };
namespace NS {
class C {};
}
S foo(S s1, NS::C c1) {
S s12{12};
using namespace NS;
C c;
}
ensuring that we pretty print that to:
struct S { int x; };
namespace NS {
class C {};
}
struct S foo(struct S s1, class NS::C c1) {
struct S s12{12};
using namespace NS;
class NS::C c;
}
(Could probably use more test coverage for other situations where the type name can appear, like within sizeof
, etc)
@AaronBallman By pretty printed you mean the use of PRETTY_FUNCTION right? |
No, I mean using the
llvm-project/clang/lib/AST/TypePrinter.cpp Line 120 in 32d16b6
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
clang/lib/AST/DeclPrinter.cpp
Outdated
@@ -897,7 +897,8 @@ void DeclPrinter::VisitFunctionDecl(FunctionDecl *D) { | |||
D->getBody()->printPrettyControlled(Out, nullptr, SubPolicy, Indentation, "\n", | |||
&Context); | |||
} else { | |||
if (!Policy.TerseOutput && isa<CXXConstructorDecl>(*D)) | |||
if (Policy.ForcePrintingAsElaboratedType || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does forcefully printing an elaborate type have to do with printing out {}
?
clang/lib/AST/TypePrinter.cpp
Outdated
if (Qualifier) | ||
OS << "class "; | ||
else | ||
OS << "struct "; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
union
? enum
?
@@ -326,6 +326,10 @@ struct PrintingPolicy { | |||
LLVM_PREFERRED_TYPE(bool) | |||
unsigned AlwaysIncludeTypeForTemplateArgument : 1; | |||
|
|||
// Whether to print the type as an elaborated type. This is used when | |||
// printing a function via the _FUNCTION__ macro in MSVC mode. | |||
unsigned ForcePrintingAsElaboratedType : 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me how this new option should interact with SuppressElaboration
and SuppressTagKeyword
or whether we should have it at all. I would have expected that the existing type printing options (like FullyQualifiedName
) would be sufficient and that we were failing to honor those in template arguments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Precommit CI has relevant failures.
clang/lib/AST/TypePrinter.cpp
Outdated
@@ -1635,6 +1635,14 @@ void TypePrinter::printElaboratedBefore(const ElaboratedType *T, | |||
if (T->getKeyword() != ElaboratedTypeKeyword::None) | |||
OS << " "; | |||
NestedNameSpecifier *Qualifier = T->getQualifier(); | |||
if (Policy.FullyQualifiedName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FullyQualifiedName
controls printing of identifier qualification, not elaboration of type names. IOW, it controls printing Bar
vs Foo::Bar
rather than B
vs struct B
.
I think you want to be looking at !SuppressTagKeyword
right?
@AaronBallman I removed the field that I added in the policy and tried to use the existing ones. That broke a few LIT tests but before fixing them I want to check with you if the combination of policy I have added in the unittest is correct. Thanks. |
ping? |
clang/lib/AST/TypePrinter.cpp
Outdated
if (Policy.SuppressTagKeyword && Policy.SuppressScope) { | ||
std::string prefix = T->isClassType() ? "class " | ||
: T->isStructureType() ? "struct " | ||
: T->isUnionType() ? "union " | ||
: ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm this still seems wrong to me. When SuppressTagKeyword
is true, tags (struct
, union
, etc) should not be printed.
clang/lib/Sema/SemaExpr.cpp
Outdated
bool ForceElaboratedPrinting = false; | ||
if (IK == PredefinedIdentKind::Function && getLangOpts().MicrosoftExt) | ||
ForceElaboratedPrinting = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bool ForceElaboratedPrinting = false; | |
if (IK == PredefinedIdentKind::Function && getLangOpts().MicrosoftExt) | |
ForceElaboratedPrinting = true; | |
bool ForceElaboratedPrinting = IK == PredefinedIdentKind::Function && getLangOpts().MicrosoftExt; |
@@ -665,7 +665,8 @@ StringRef PredefinedExpr::getIdentKindName(PredefinedIdentKind IK) { | |||
// FIXME: Maybe this should use DeclPrinter with a special "print predefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The more complex we make this, the more I think this FIXME sounds like a somewhat better idea though it would potentially be an involved refactoring...
clang/lib/AST/Expr.cpp
Outdated
const auto &LO = Context.getLangOpts(); | ||
if (ForceElaboratedPrinting) { | ||
if ((IK == PredefinedIdentKind::Func || | ||
IK == PredefinedIdentKind ::Function) && | ||
!LO.MicrosoftExt) | ||
return FD->getNameAsString(); | ||
if (IK == PredefinedIdentKind::LFunction && LO.MicrosoftExt) | ||
return FD->getNameAsString(); | ||
} else { | ||
if (IK != PredefinedIdentKind::PrettyFunction && | ||
IK != PredefinedIdentKind::PrettyFunctionNoVirtual && | ||
IK != PredefinedIdentKind::FuncSig && | ||
IK != PredefinedIdentKind::LFuncSig) | ||
return FD->getNameAsString(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are all doing the same thing -- calling FD->getNameAsString()
; can we simplify the logic?
clang/lib/AST/TypePrinter.cpp
Outdated
Policy, TPL, ParmIndex)); | ||
if (!Policy.SuppressTagKeyword && | ||
Argument.getKind() == TemplateArgument::Type && | ||
!Argument.getAsType()->isBuiltinType()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be looking at isa<TagType>(getAsType())
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The failure found by CI looks to be related:
<stdin>:422:1: note: possible intended match here
VarDecl:{ResultType struct struct X}{TypedText f1} (50) (deprecated)
(note the struct struct
).
Interesting! The LIT test is passing on a debug build on Windows and generating the right output:
I will try a release build. |
It looks like I can reproduce it with the release build. |
Fixes the `sanitizer-x86_64-linux-android` buildbot.
These were last used in the fcopysign lowering, which now uses AArch64ISD::BSP.
If the buildvector node contains extract, which later should be combined with some other nodes by shuffling, need to estimate the cost of this shuffle before building the mask after shuffle. Reviewers: RKSimon Reviewed By: RKSimon Pull Request: llvm#83442
This adds operations for bitwise operators. Furthermore, an UnaryOp class and a helper to print unary operations are introduced.
When do the analysis for the (potential) masked gather node, we check that not greater than half of the pointer operands are loop invariants or potentially vectorizable. Need to check actually, that we have a loop at first and do better check for the potentially vectorizable pointers. Reviewers: RKSimon Reviewed By: RKSimon Pull Request: llvm#83472
Fix typo introduced by 6e36ceb
…ow their own template parameters (llvm#78274)" (llvm#79683) Reapplies llvm#78274 with the addition of a default-error warning (`strict-primary-template-shadow`) that is issued for instances of shadowing which were previously accepted prior to this patch. I couldn't find an established convention for naming diagnostics related to compatibility with previous versions of clang, so I just used the prefix `ext_compat_`.
…unctions (second try). (llvm#80457) Default value of checker option `ModelPOSIX` is changed to `true`. Documentation is updated. This is a re-apply of commit 7af4e8b that was reverted because a test failure (this is fixed now).
The class `CallDescription` is used to define patterns that are used for matching `CallEvent`s. For example, a `CallDescription{{"std", "find_if"}, 3}` matches a call to `std::find_if` with 3 arguments. However, these patterns are somewhat fuzzy, so this pattern could also match something like `std::__1::find_if` (with an additional namespace layer), or, unfortunately, a `CallDescription` for the well-known function `free()` can match a C++ method named `free()`: llvm#81597 To prevent this kind of ambiguity this commit introduces the enum `CallDescription::Mode` which can limit the pattern matching to non-method function calls (or method calls etc.). After this NFC change, one or more follow-up commits will apply the right pattern matching modes in the ~30 checkers that use `CallDescription`s. Note that `CallDescription` previously had a `Flags` field which had only two supported values: - `CDF_None` was the default "match anything" mode, - `CDF_MaybeBuiltin` was a "match only C library functions and accept some inexact matches" mode. This commit preserves `CDF_MaybeBuiltin` under the more descriptive name `CallDescription::Mode::CLibrary` (or `CDM::CLibrary`). Instead of this "Flags" model I'm switching to a plain enumeration becasue I don't think that there is a natural usecase to combine the different matching modes. (Except for the default "match anything" mode, which is currently kept for compatibility, but will be phased out in the follow-up commits.)
This patch is a minor formatting fix in the OpenMP ops documentation wherein the examples section in ``omp.private`` was breaking out into a top level list item.
…ed Vectors (llvm#83038) Legalize smaller/larger than legal vectors with i8 and i16 element sizes. Vectors with elements smaller than i8 will get widened to i8 elements.
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