JonasToth added a comment. > Using regex could be a reasonable way out, but isn't going to solve the > problem in general because not all of the macro names are ones the user has > control over. Having to whitelist dozens of macros by name means this check > is simply going to be disabled by the user as being low-value.
Yes. I think with the regex its exactly implemented like the rules say. But adding sugar is still reasonable. I will run the check over llvm to get a feeling how much is still left after silence `LLVM_*` and others that are clearly scoped. > Except that's not the only form of conditional compilation I'm worried about. > See Compiler.h where we do things like `#define __has_attribute(x) 0`, which > is a perfectly reasonable macro to define (and is an example of a macro name > outside of the user's control, so they cannot simply prefix it for a > whitelist). Having a regex allows to add `__has_attribute` to it or a pattern like `__has*`. But how many cases for such macros are left? The goal of the guidelines is to change coding style and requiring to silence _SOME_ warnings is reasonable to me. > However, I have no idea how we would handle cases like LLVM_LIKELY and > LLVM_UNLIKELY, but expecting users to NOLINT 500+ lines of common macro code > doesn't seem like a good first approach. I'd be curious to know what the C++ > Core Guidelines folks think about those use cases and how they would > recommend rewriting the code to adhere to their guidelines. Do they really > expect users to use *no* macros in C++ code outside of header include guards > and noop definitions even when asked about reasonable use cases like > cross-compiler attributes or builtins? I mean, many of these things cannot > even use the attribute the C++ Core Guidelines require for silencing such > diagnostics -- how do they want to use gsl::suppress to silence a diagnostic > on a macro definition? I think the long term goal is removing the preprocessor, but especially compiler specifics are very unlikely to get away soon. They themself make exceptions: ES.33: If you must use macros, give them unique names <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es33-if-you-must-use-macros-give-them-unique-names> >> 2. Compiler intrinsics These are similar to attributes and could maybe even >> treated the same. > > I'm less certain about this one -- with attributes, there's syntax that we > can inspect (is there an `__attribute__` or `__declspec` keyword?), but with > intrinsics there's no commonality. Or are you thinking of tablegenning the > list of intrinsics? I thought mostly of `__builtin` when proposing. Checking the MSVC docs showed that not all compilers do follow such nice naming conventions. Adding a clang and gcc list of builtins might still be manageable with basic string handling. I feel that keeping any list of all compiler intrinsics is too much. >> 3. Compatibility with older C++ Standards Stuff like `#define OVERRIDE >> override` if the code is compiled with C++11 or newer might be acceptable >> for stepwise modernization. This can can be considered as program text >> manipulation that was explicitly forbidden but does not obscure code so an >> exception could be made. Such macros can be detected with string comparisons. > > How would you generalize this? I've seen people use macros for arbitrary > keywords (`#define CONST const` and `#define false false`) as well as macros > for arbitrary syntax (`#define DEFAULTED {}` or `#define DEFAULTED = > default`). Not sure about all. I think new keywords like `#define OVERRIDE override` should definitly be allowed. But things like `false` not, because it already is a keyword in C++98. I am against allowing `#define DEFAULTED` because both are perfectly valid and we ship a check for modernizing from one to the other version. This is different to `OVERRIDE`, because it adds additional value for the programmer who might be stuck at C++98. >> 4. Debugging/Logging stuff that contains the Line and file. These can not be >> modeled with current C++(?), but `LineInfo` or similar exists. I don't know >> what its status is. I think it is ok to require manual silencing for such >> macros. > > In addition to `__LINE__` and `__FILE__`, I think we want to include > `__DATE__`, `__TIME__`, and `__func__`. Agreed. > I think this list is a good start, but is likely incomplete. We'll probably > discover other things to add to the list when testing the check against other > real world code bases to see what patterns emerge from them. Testing Frameworks are a common source for macro usage too. They should be coverable with the regex approach, but I don't know if there are other things hidden behind the interface that would be super annoying to silence. Same for benchmark libraries. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41648 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
