aaron.ballman added a reviewer: aaron.ballman. aaron.ballman added a comment.
Thank you for the patch! It looks like it's missing test coverage for the changes, can you add some to `clang/test/Preprocessor`? One question I have is: if we're going to support `__FUNCTION__`, shouldn't we also support `__FUNCDNAME__` and `__FUNCSIG__` as well, as those are also Microsoft predefined macros in the same general space? ================ Comment at: clang/include/clang/Lex/Preprocessor.h:153 /// Identifiers for builtin macros and other builtins. - IdentifierInfo *Ident__LINE__, *Ident__FILE__; // __LINE__, __FILE__ + IdentifierInfo *Ident__LINE__, *Ident__FILE__, *Ident__FUNCTION__; // __LINE__, __FILE__, __FUNCTION__ IdentifierInfo *Ident__DATE__, *Ident__TIME__; // __DATE__, __TIME__ ---------------- You should put your declaration on a new line so the line length doesn't exceed 80 columns (it's part of our coding style: https://llvm.org/docs/CodingStandards.html). ================ Comment at: clang/lib/Lex/PPMacroExpansion.cpp:340-344 +#ifdef _WIN32 + Ident__FUNCTION__ = RegisterBuiltinMacro(*this, "__FUNCTION__"); +#else + Ident__FUNCTION__ = Ident__LINE__ = RegisterBuiltinMacro(*this, "__LINE__"); +#endif ---------------- This looks incorrect in a few ways. The first is that you shouldn't use `#if _WIN32` for this -- that means the compiler, when compiled for Win32, will have `__FUNCTION__` which isn't what you were trying for (you'd want to use the language options to determine if you're compiling for Win32 or not, regardless of what OS the host compiler is running on). Also, in the `#else` block you are registering `__LINE__` as the identifier for `__FUNCTION__`, which isn't correct. However, if we're going to support `__FUNCTION__`, we should support it on all targets instead of just Windows, so I don't think you need a conditional at all here. ================ Comment at: clang/lib/Lex/PPMacroExpansion.cpp:1569-1575 +#ifdef _WIN32 + if (II == Ident__FUNCTION__) { + FN += "("; + FN += Twine(PLoc.getLine()).str(); + FN += ") "; + } +#endif ---------------- You don't want the `#ifdef` here either, but also, this looks incorrect compared to the output you get from MSVC. `__FUNCTION__` is the unmangled name of the function without any parameter or return type information, and this is printing line information. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136343/new/ https://reviews.llvm.org/D136343 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
