I see. Thank you for fixing + explaining :)

• fix bad template instantiation

I migrated to the new Decl interface and adjusted my tests, there are no false positives left and I am not aware of more possible code constellation that would require testing.
If you have time you could check my tests, maybe you find something I missed.

• migrate to Decl interface
• add template metaprogramming test

I will migrate to the new API today evening (european time).

• [Feature] use new statefull const checker
• [Test] add more tests finding false positives

There is a false positive with the reference checking. Could you please take a look at it? I could not find the reason from inspecting your code.

I like your refactoring very much and i think we have a state that is close to commit.

Something came to my mind:

• [Misc] mark false positives

@shuaiwang I implemented the check on top of you utility function. It does fail right now (concentrating on values only for now). I added a FIXME for all false positives/negatives for the values test cases.

• [Feature] refactor check to use a utility function to determine constness

Another note: https://github.com/llvm-mirror/clang-tools-extra/blob/master/clang-tidy/utils/DeclRefExprUtils.cpp
There is a isOnlyUsedAsConst with a slick implementation, using a set difference.

I was thinking about that too.

OpenCV isn't clean either, here the results:

Filter: ASSERT*|ALL*|CAL*|CC*|CALC*|calc*|CL*|cl*|CUDA*|CV*|cv*|EXPECT*|GTEST*|FUNCTOR*|HAVE*|ICV*|IPL*|IPP*|ipp*|__itt*|ITT*|JAS*|jas*|MESSAGE*|MAX*|OCL*|opengl*|OPENCV*|TYP*

This one worries me a bit more because of patterns like cl* and calc* -- those seem like they're not uncommon and the pattern may silence otherwise valid diagnostics.

You are doing a great job and i learn new stuff :)

I will check this one first, before we get crazy and implemented something twice.

Short note here too: I think having isModified return a track record, like a vector for each

1. modifications
2. non-const handle taking
3. (const usages)

would be nice. Every user can decide how to react to it. Then the function would be more like usageRecord with pointers to each usage.
That allows graph building, it allows detailed diagnostics and contains all relevant information for the expr.

In D45444#1069488, @shuaiwang wrote:

I've updated https://reviews.llvm.org/D45679 and I think the isModified() function there is now sufficiently covering the cases you've covered here and can be used as a good starting version if you plan to use it here.
I copied your const-values test cases and it now passes with the following differences:

• All unused local variables removed, my check will issue a warning for them because technically they're not modified, but I understand why you don't want to cover them. I don't feel strongly about it and I removed it from my copy-pasted test cases because I just to demonstrate isModified()

You'll also need to check:

• a member function calling another non-const member function -> *this can't be const
• *this is passed as non-const reference param to a function -> *this can't be const

Coming from https://reviews.llvm.org/D45679, which I should have sent out much earlier to get in front of you :p

Kidding aside this check is more mature than mine and I'm happy to help incorporate mine into this one.

Hi @shuaiwang, i am currently working on the same check and we should definilty coordinate our work. :)

@aaron.ballman @lebedev.ri The check is getting really complex. I run it over LLVM and detected some warnings, where iam not sure if they are valid or not. Its already a somewhat usable state, but its hard to determine false positives and false negatives.

• [Misc] unify handle and value modification detection
• [Misc] found new caveats, maybe take a look at refactoring first an require this check to produce clean compiles on llvm

In D41648#1047799, @JonasToth wrote:

I checked several codebases and implemented a little helper script to see which kind of macros exist. Then I added some filter regex as configuration and tried again, here are the results:

For https://github.com/nlohmann/json which is a very high quality codebase, the results were as expected: very clean, and good macro usage: (except a #define private public for tests)

Filter: JSON*|NLOHMANN*

all_macros.txt695 B

Download

filtered_macros.txt47 B

Download

CMake on the other hand uses many macros and don't obey CAPS_ONLY rules for all macros.

Filter: _FILE_OFFSET_BITS|AE_*|ARCHIVE_*|cal_*|CMAKE_*|cm*|CM_*|CPP_*|CURL*|E_*|exp_*|F90*|jp*|JSON_*|KW*|kw*|O_*|REQ_*|UV_*|YY*|yy*|Z_*

warned_macros.txt18 KB

Download

filtered_macros.txt3 KB

Download

OpenCV isn't clean either, here the results:

Filter: ASSERT*|ALL*|CAL*|CC*|CALC*|calc*|CL*|cl*|CUDA*|CV*|cv*|EXPECT*|GTEST*|FUNCTOR*|HAVE*|ICV*|IPL*|IPP*|ipp*|__itt*|ITT*|JAS*|jas*|MESSAGE*|MAX*|OCL*|opengl*|OPENCV*|TYP*

all_macros.txt276 KB

Download

filtered_modules_macros.txt13 KB

Download

libtorrent https://github.com/arvidn/libtorrent has reasonable macro usage

Filter: DEBUG*|LIBTORRENT*|TORRENT*|UNI*

filtered_macros.txt1 KB

Download

all_macros.txt5 KB

Download

LLVM lib/ defines many macros, almost all of them are ALL_CAPS

Filter: AARCH64*|X86*|ARM*|AMDGPU*|ASAN*|ATTR*|CHECK*|COMMON*|CV*|CXX*|DEBUG*|DEC*|DEFINE*|ESAN*|ENUM*|FMA*|FUZZER*|HANDLE*|INTERCEPTOR*|LSAN*|MATH*|MSAN*|OPENMP*|TSAN*|TYPE*|UBSAN*

all_lib_macros.txt83 KB

Download

filtered_lib_macros.txt10 KB

Download

LLVM tools/ is a similar story
Filter: ANALYSIS*|ASSERT*|CHECK*|DEBUG*|DECL*|DEPENDENT*|DIAG*|END*|ENUM*|GENERIC*|IMAGE*|KEYWORD*|LANG*|LLVM*|NON*|OBJC*|OPENMP*|OVERLOAD*|PROC*|REGISTER*|SANITIZER*|STMT*|TYPE*|X86*

all_tools_macros.txt26 KB

Download

filtered_tools_macros.txt6 KB

Download

@aaron.ballman Would you like to see other projects checked? I think this is a starting point for now.

My opinion based on what i see is

• maybe two modes for this check make sense, having one requiring CAPS_ONLY and the currently implemented stricter check. This allows easier migration to safer macro usage
• it is possible to reduce macro usage to a minimal amount, and the complex macros like AST stuff can be filtered with the regex. Furthermore, restricting all macros to a "macro namespace" is possible for sure.
  
  Things I would like to add to the check:
• my little filtering script is valuable for developers, that want to address the macro issue. It should be added to the docs and everyone can make something based on that. It will be linux centered.
• enforcing ALL_CAPS, including its usage
• (adding a prefix to all macro, passing the filter, including its usage)
  
  Code transformation has the problems of scope and potentially breaking code badly, because clang-tidy wasn't run over all of the code.

The check is chatty, as expected, because macros in header files, especially central ones, pollute everything. That is the reason for the check, too.
Overall I am still in favor of this approach, seeing some obscure macros that should be replaced with actual language features (like overloading, .inline functions, constants, ...) in the checked codebases.

• [Feature] start working on handle semantic

• [Feature] implement array and method access
• [Feature] finalize value semantic

Commit in https://reviews.llvm.org/rCTE329789

• remove shifting exception for standarized bitmask types

It'll be good idea to have option to apply this check for pointer/references only, or include built-in types/enums.

Sure. Is it OK to add a dependency from clang-tidy/bugprone/BugproneTidyModule.cpp to stuff in clang-tidy/cpp-coreguidelines ? Is there an existing example of this ?

@aaron.ballman, @lebedev.ri, @alexfh and everyone else, i would love to hear your opinion on my approach. i never implemented a similar check and i might miss some cases, that need to be considered.

Could you please add some tests that include user defined literals and there interaction with other literals. We should catch narrowing conversions from them, too.

Is there anything I need to do for the diff to change state ? I

thought updating the code would automatically mark it "ready for review"
again.

That sounds good.

my 2 cents:

• fix std bitmask type shifting issue

LGTM, but @aaron.ballman, @alexfh or someone else should review it before comitting.

Alright. Thank you for clarification :)

I'm not quite sure how we would go about that with confidence. The code we'd need to catch looks something like:

typeof(foo() == y) x;
/* snip */
bar(x == -1); // warning: comparison is pointless

If we could tell that x's type was inferred from a comparison op, we could warn here. However, if the user used x as anything but a C++ bool in /* snip */, the warning would be incorrect. We could maybe do some sort of range analysis by walking the AST, but that sounds like more like a static analyzer thing.

...And none of that would catch glibc's TEMP_FAILURE_RETRY macro, unless we wanted to apply said analysis to all integral variables.

You noted, that the C++ warning would catch this case, but does not get triggered in C. Would it be wort to generalize the concern and have a warning that catch the comparison, regardless of the macro?

Which checks do you have in mind?

• [Feature] implement CAPS_ONLY features, adjust docs

I checked several codebases and implemented a little helper script to see which kind of macros exist. Then I added some filter regex as configuration and tried again, here are the results:

• Merge branch 'master' into check_macros_usage
• [Doc] update to new doc style

• update to trunk
• [Doc] update to new doc style

I think this check could land in the bugprone module.

• add fixme for degenerated switch

• adress review comments

• remove spurious debug iostream

I think the check is ready to land. I did check run it over LLVM and found some interesting code parts that could benefit. I extracted all the warnings and sorted them into the categories missing default/covered codepath(uncovered.txt), better use if/else(better_if.txt) and use if(mandatory_if.txt). Some warnings come from generated files but some live in usercode. In total there are 540 warnings.

• reorder check in release notes to get it in alphabetical order

• get the check working again
• enable the big test

Good news: The stack overflow seems to be fixed, by the patch r326624 from @rsmith (Thank you very much!).
Bad news: The check does not do its job anymore.... I try to fix the functionality problems.

The comment i added is actually wrong.
The range-based for just works anyway.
The VisitBindingDecl() is needed to get all the 'variables' declared in structured binding.
But as you can see in https://godbolt.org/g/be6Juf, the BindingDecl's are wrapped into DecompositionDecl.
And DecompositionDecl is actually inherited from VarDecl, https://github.com/llvm-mirror/clang/blob/b031fdc9b7dbd9c7f942d8060a4f00d63f3c9af2/include/clang/AST/DeclCXX.h#L3824-L3825,
so we count the DecompositionDecl in VisitVarDecl(), thus we need a VisitDecompositionDecl() that subtracts it.

Looks fine to me, but Aaron or someone else must accept :)

The check is related to: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#cp44-remember-to-name-your-lock_guards-and-unique_locks

After long inactivity (sorry!) i had a chance to look at it again:

The guideline authors will focus on mixing integer length and integer signdness.