- User Since
- Oct 31 2016, 11:13 AM (81 w, 5 d)
Fri, May 4
Wed, May 2
I see. Thank you for fixing + explaining :)
Mon, Apr 30
- 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).
Sat, Apr 28
- [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.
Apr 20 2018
I like your refactoring very much and i think we have a state that is close to commit.
Something came to my mind:
Apr 18 2018
- [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:
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 :)
Apr 17 2018
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
- non-const handle taking
- (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.
Apr 16 2018
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. :)
Apr 15 2018
@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
Apr 14 2018
- [Feature] start working on handle semantic
- [Feature] implement array and method access
- [Feature] finalize value semantic
Apr 11 2018
Commit in https://reviews.llvm.org/rCTE329789
- remove shifting exception for standarized bitmask types
Apr 10 2018
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 ?
Apr 9 2018
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"
That sounds good.
my 2 cents:
Apr 8 2018
- fix std bitmask type shifting issue
Apr 2 2018
Mar 31 2018
Alright. Thank you for clarification :)
Mar 30 2018
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?
Mar 27 2018
Mar 26 2018
Which checks do you have in mind?
- [Feature] implement CAPS_ONLY features, adjust docs
Mar 25 2018
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:
Mar 24 2018
- Merge branch 'master' into check_macros_usage
- [Doc] update to new doc style
- update to trunk
- [Doc] update to new doc style
Mar 22 2018
I think this check could land in the bugprone module.
Mar 21 2018
- add fixme for degenerated switch
- adress review comments
Mar 18 2018
- 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 :)
Mar 8 2018
Mar 1 2018
After long inactivity (sorry!) i had a chance to look at it again:
Feb 6 2018
Jan 31 2018
Jan 22 2018
The guideline authors will focus on mixing integer length and integer signdness.