[PATCH] D153914: [clang-cl] Enable concatenation of predefined identifiers

[Richard Dzenis via Phabricator via cfe-commits]

RIscRIpt added inline comments.


================
Comment at: clang/include/clang/Basic/TokenKinds.h:87-93
+/// Return true if this token is a predefined macro
+/// unexpandable by MSVC preprocessor.
+inline bool isUnexpandableMsMacro(TokenKind K) {
+  return K == tok::kw___FUNCTION__ || K == tok::kw___FUNCSIG__ ||
+         K == tok::kw_L__FUNCTION__ || K == tok::kw_L__FUNCSIG__ ||
+         K == tok::kw___FUNCDNAME__;
+}
----------------
tahonermann wrote:
> cor3ntin wrote:
> > tahonermann wrote:
> > > RIscRIpt wrote:
> > > > tahonermann wrote:
> > > > > 
> > > > Thanks, I like the name. But shouldn't we reflect that we are referring to only Microsoft (unexpandable) macros? How about `isFunctionLocalPredefinedMsMacro`?
> > > I don't think the Microsoft association is meaningful. Sure, some of the predefined macros are only treated differently when compiling in Microsoft mode, but some of those macros aren't Microsoft specific. Also, there might be macros provided by other implementations that we'll want to emulate some day.
> > I think it is, there is currently no way to link `isFunctionLocalPredefinedMacro` to the MS feature. "MSPredefinedMacro" is pretty self explanatory, same reason we most often - but not always - use GNU in the name of function related to GNU extensions.
> > There are enough similar-sounding features and extensions that we should try to make it clear which feature we are talking about.
> Perhaps we still haven't found the right name then. With the name that I've been advocating, this function should also return true for `tok::kw___PRETTY_FUNCTION__` even though that macro should not expand to something that can be concatenated with other string literals (whether compiling in Microsoft mode or not).
> 
> The Microsoft extension is the localized expansion to a string literal that can be concatenated with other string literals. We probably should isolate the conditions for that behavior to one place and if we do that, then I guess naming this function as specific to Microsoft mode is ok; we can always rename it later if it gets used for non-Microsoft extensions.
> 
> Here is my suggestion then:
>   // Return true if the token corresponds to a function local predefined
>   // macro that, in Microsoft modes, expands to a string literal (that can
>   // then be concatenated with other string literals).
>   inline bool isMsFunctionLocalStringLiteralMacro(TokenKind K, const LangOptions &langOpts) {
>     return langOpts.MicrosoftExt && (
>         K == tok::kw___FUNCTION__ || K == tok::kw_L__FUNCTION__ ||
>         K == tok::kw___FUNCSIG__ || K == tok::kw_L__FUNCSIG__ ||
>         K == tok::kw___FUNCDNAME__);
>   }  
> 
> This will avoid the need for callers to check for `MicrosoftExt`; that is what I really want to avoid since it is easy to forget to do that check.
1. By requiring user to pass `LangOptions` I think we can remove MS association (implying that there could be other non-msft cases in the future)
2. We would have to include a `LangOptions.h` in `TokenKinds.h`, are we ok with this? Alternatively while this function is for msft cases only, we could pass `bool MicrosoftExt`

Personally, I like version with `LangOptions` and removal of `MS`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153914/new/

https://reviews.llvm.org/D153914