MSVC makes these string literals [1][2].
[1] https://godbolt.org/z/6vnTzbExx
[2] https://learn.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=msvc-170
Fixes #114
Differential D146764
[clang] Make predefined expressions string literals under -fms-extensions aeubanks on Mar 23 2023, 3:18 PM. Authored by
Details MSVC makes these string literals [1][2]. [1] https://godbolt.org/z/6vnTzbExx Fixes #114
Diff Detail
Unit Tests Event TimelineComment Actions +mstorsjo is this okay for mingw mode too?
Comment Actions This should also have a release note, eventually.
Comment Actions I believe the current form of the patch is fine - i.e. disabled by default in mingw mode, but enabled if the extra MS language extensions are enabled. (I didn't check myself how those options map to LangOpts.MicrosoftExt but I presume it works as I described above.)
Comment Actions I think you're missing changes in ASTReaderStmt.cpp and ASTWriterStmt.cpp, so serialization through modules or PCH won't work without that.
Comment Actions Thanks! Typically with a test using modules or PCH, so something along these lines: https://github.com/llvm/llvm-project/blob/main/clang/test/Modules/cxx20-10-5-ex1.cpp and/or https://github.com/llvm/llvm-project/blob/main/clang/test/PCH/cxx-namespaces.cpp
Comment Actions added the warning, not sure if this needs more tests for testing the interaction between -pedantic, -Wmicrosoft, -Wmicrosoft-init-from-predefined or if that's already assumed to work
Comment Actions That's generally assumed to work, really the only question is whether we want Extension (only warned in -pedantic) or ExtWarn (warned by default). I think most of our Microsoft extension warnings are ExtWarn and it seems defensible for this to be on by default, but it'd be nice to know just how chatty this is on real world code bases too. I have the sneaking suspicion this code pattern doesn't come up often enough for this to be "too chatty", but if we find it triggers on popular libraries we might want to consider dialing it back to just Extension.
Comment Actions Precommit CI found some related failures that need to be addressed, but the functional changes are all looking good to me. Be sure to also add a release note for the fix as well. Comment Actions Presuming that the issues found by precommit CI were resolved (current failures are unrelated to your changes), this LGTM but still needs a release note. Comment Actions the following crashes with this patch: struct StringRef { StringRef(const char *); }; template <typename T> StringRef getTypeName() { StringRef s = __func__; } clang -cc1 -fms-extensions -std=c++17 -x c++ a.cpp -fsyntax-only Comment Actions Sorry for the late comments.
|
I think we should keep PredefinedExpr general, so how about IsTransparent instead? Then we can add a comment above Create() that explains what IsTransparent does.
(This makes it easier for us to add predefined expressions that aren't string literals but are still transparently handled.)