Page MenuHomePhabricator

[clang-tidy] Assert related checkers
ClosedPublic

Authored by eszasip on Feb 3 2015, 9:05 AM.

Details

Reviewers
alexfh
Summary

This patch contains two assert related checkers. These checkers are the part of those that is being open sourced by Ericsson (http://lists.cs.uiuc.edu/pipermail/cfe-dev/2014-December/040520.html).

The checkers:

AssertSideEffect:
/ \brief Finds \c assert() with side effect.
/
/ The conition of \c assert() is evaluated only in debug builds so a condition
/ with side effect can cause different behaviour in debug / relesase builds.

StaticAssert:
/ \brief Replaces \c assert() with \c static_assert() if the condition is
/ evaluatable at compile time.
/
/ The condition of \c static_assert() is evaluated at compile time which is
/// safer and more efficient.

Diff Detail

Event Timeline

eszasip updated this revision to Diff 19232.Feb 3 2015, 9:05 AM
eszasip retitled this revision from to [clang-tidy] Assert related checkers.
eszasip updated this object.
eszasip edited the test plan for this revision. (Show Details)
eszasip added a reviewer: alexfh.
eszasip added subscribers: Unknown Object (MLST), xazax.hun.
alexfh edited edge metadata.Feb 4 2015, 11:35 AM

Nice checks! Could you run it over LLVM/Clang and see whether they finds something?

Also, a couple of comments below.

clang-tidy/misc/AssertSideEffectCheck.cpp
22

This one is not needed, as the rest of the file is inside namespace clang {}. Let's just leave using namespace clang::ast_matchers; here.

34

AFAIU, RetVal can never be changed below. So why not just return here? Same is valid for other cases.

92

Does it work if you pull out the common part:

auto ConditionWithSideEffect = 
  hasCondition(hasDescendant(expr(hasSideEffect(CheckFunctionCalls)))));
Finder->addMatcher(
      stmt(anyOf(conditionalOperator(ConditionWithSideEffect), ifStmt(ConditionWithSideEffect))));

?

110

nit: Please finish statements with a period It makes comments look nicer

test/clang-tidy/misc-assert-side-effect.cpp
12

Can you add a conditional operator-based implementation for testing?

eszasip updated this revision to Diff 19763.Feb 11 2015, 8:42 AM
eszasip edited edge metadata.

Fixed style issues. Added a conditional operator-based assert() to misc-assert-side-effect.cpp.

StaticAssertCheck:
-added new FixIt cases: assert((...) && "AssertMsg") --> static_assert((...), "AssertMsg")
-added test cases for each FixIt case

Thanks for addressing the comments. A few comments more below and now I managed to review the second check as well.

clang-tidy/misc/AssertSideEffectCheck.cpp
45
  1. Why do you need to place this inside the if (const auto *CExpr = llvm::dyn_cast<CallExpr>(E)) {?
  1. I'd prefer to put the definition inside the condition:

    if (const auto *OpCallExpr = llvm::dyn_cast<CXXOperatorCallExpr>(E)) { ...
50

Two comparisons with OO_SlashEqual.

57

Maybe s/RetVal/Result/? It's the same length, but it's a word.

59

MethodDecl is just two letters more, but reads better.

64

Here and elsewhere: I think, dyn_cast, isa and StringRef are imported in either clang or global namespace. Please check if the code compiles without explicit qualifiers.

75

Why is this option needed? Why "false" is the right default value?

76

The fact that it's a list can be conveyed by just using a plural, e.g. AssertMacros.

78

It seems that comma-separated lists are more common.

clang-tidy/misc/StaticAssertCheck.cpp
71

s/ASTCtx->getSourceManager()/SM/

84

I'd replace these with a SmallVector<FixItHint, 4>.

91

I think, in case there's a message, the fixes should be made as follows:

  • insert a comma right after the end of the LHS (matters if there is a line break or a comment after the LHS)
  • remove the "&&" and any whitespace after it
  • don't touch the message at all.

In case there is no message originally, just insert a ", \"\"" right before the closing brace.

WDYT?

108

Why do you care about retention of comments? Does it matter?

110

Do you expect to meet anything between "assert" and "("? Maybe just use Lexer::getLocForEndOfToken and start the search of parentheses from after the "assert" token?

114

Please pull the search of matching parentheses in a function. Probably, together with the Lexer, Buffer and Token variables.

117

nit: I'd use else if to emphasize that these cases are mutually exclusive.

clang-tidy/misc/StaticAssertCheck.h
2

It's fine for now, but generally I'd prefer if you sent one patch per check. It would be easier to review and the review of each patch wouldn't block the other one.

Thanks!

LegalizeAdulthood added inline comments.
clang-tidy/misc/AssertSideEffectCheck.h
24

condition misspelled

26–31

I always find it odd when reading a class to be shown its internal details before I'm shown its public API. I guess it saves having to write a single line private:, but it makes it more distracting to read the source code.

clang-tidy/misc/AssertSideEffectCheck.cpp
47–54

What about OO_AmpEqual?

test/clang-tidy/misc-assert-side-effect.cpp
43–63

Most of the items detected have no test cases. I used the FileCheck tests to push my implementation forward ensuring that I always had some test case for all the combinations. The test cases can also serve as good executable documentation for everything the check does.

eszasip updated this revision to Diff 20017.Feb 16 2015, 5:28 AM

Added:
--more test cases to AssertSideEffectChecker
--comments explaining the options of AssertSideEffectChecker

Fixed everything that alexfh and LegalizeAdulthood suggested except the "whitespace before the comma" problem.

clang-tidy/misc/AssertSideEffectCheck.cpp
47–54

That's what I wanted to write instead of the second OO_SlashEqual.

clang-tidy/misc/StaticAssertCheck.cpp
91

I also thought about this solution but it would generate incorrect source code if the argument of assert was in parentheses or "&&" had its operands swapped. These cases might be rare but my main aim was not to generate incorrect code.

About fixing the 'comma problem' while keeping the genericness:
I could have ignored the second case or written an even bigger matcher (and FixIt).
In the first case, distinguishing between parentheses in the argument or in the macro definition and deciding where the remaining whitespaces belong would make the code so complicated that I thought it didn't worth it.

If you suggest generating a nice-looking source code and risking that it will not always complie, I will change that part.

Sorry for the long delay. This looks better, but still a few comments.

clang-tidy/misc/AssertSideEffectCheck.cpp
45

So why do you need to place this if inside the if (.. = dyn_cast<CallExpr>(E))?

clang-tidy/misc/AssertSideEffectCheck.h
25

nit: The comment is now mis-formatted.

29

How about making the comment a bit less wordy:

  • AssertMacros: A comma-separated list of the names of assert macros to be checked.

?

33

This doesn't explain much. "Checking function calls" is not what this option controls. AFAIU, it makes the check treat non-const member calls as though they produce side effects.

I also don't understand what do you mean by "reporting free functions". Maybe add a motivating example?

clang-tidy/misc/StaticAssertCheck.cpp
84

Creating an empty vector and then using push_back() to add whatever fixes you need looks like a more elegant solution.

104

Starting from r230495, you can insert FixItHints directly.

108

As I probably mentioned before, top-level constness doesn't make sense in function parameter types. It is just an implementation detail that is not interesting to the API users, but it makes the interface less readable.

test/clang-tidy/misc-assert-side-effect.cpp
2

Please remove the empty line.

eszasip updated this revision to Diff 20677.Feb 25 2015, 8:10 AM

Fixed the problems alexfh pointed out.

alexfh added inline comments.Feb 25 2015, 10:43 AM
clang-tidy/misc/StaticAssertCheck.cpp
55

nit: Do you need ASTCtx to point to a non-const ASTContext?

73

I wonder if it's intentional that the other check has an option for assert macro names and this one doesn't.

109

Do you need ASTCtx to point to a non-const ASTContext? I only dislike top-level const ;)

Also, please remove the "const" on the second parameter.

test/clang-tidy/misc-assert-side-effect.cpp
46

This comment doesn't seem to be very useful.

xazax.hun added inline comments.Feb 26 2015, 12:30 AM
clang-tidy/misc/StaticAssertCheck.cpp
73

I think the fixit might not be valid for a custom assert. Once custom assert macros are supported either those should not be rewritten or it should be configurable whether to rewrite custom macros or not. Or we can just rely on that the users won't run this checker with custom asserts configured and apply fixits mode. What do you think?

alexfh added inline comments.Feb 26 2015, 1:25 AM
clang-tidy/misc/StaticAssertCheck.cpp
73

Makes sense, the fixits may be incorrect for custom asserts. One possible example is:

CHECK(some_boolean_expression) << "Error message: " << some_parameter;

Another couple of possible solutions:

  • configure a separate list of macros that the check should not issue fixits for;
  • try to detect automatically whether we can handle the specific construct (e.g. if it's in the form of macro(expression);).

In any case it looks like it's better to do this in a separate patch.

eszasip updated this revision to Diff 20850.Feb 27 2015, 6:53 AM
This comment was removed by eszasip.
eszasip updated this revision to Diff 20851.Feb 27 2015, 6:58 AM

Moved const qualifier to the right place.

alexfh accepted this revision.Feb 27 2015, 9:31 AM
alexfh edited edge metadata.

Looks good! (with one nit, see the comment)

Thanks for the contribution!

Do you need me to commit the patch for you?

clang-tidy/misc/AssertSideEffectCheck.cpp
97

nit: Remove the space after the star.

This revision is now accepted and ready to land.Feb 27 2015, 9:31 AM
eszasip updated this revision to Diff 20871.Feb 27 2015, 12:38 PM
eszasip edited edge metadata.

Removed the space after the star.

Please commit this patch for me.

Thanks for your help so far.

alexfh closed this revision.Mar 2 2015, 2:52 AM

Committed revision 230943.

A few changes before commit:

  • Fixed a couple of issues I noticed (see the comments).
  • Removed some top-level consts for consistency with the rest of clang-tidy code (and other reasons I mentioned earlier in reviews).

Thank you for contributing these checks!

clang-tidy/misc/AssertSideEffectCheck.cpp
76

nit: This can be a temporary.

clang-tidy/misc/AssertSideEffectCheck.h
30

This part of the comment is redundant since we already said that this is a comma-separated list.

I found one issue with the misc-static-assert check. The check warns on assert(false) in some configurations. I've filed http://llvm.org/PR22880.

Could you take a look at the issue?

lebedev.ri added inline comments.
test/clang-tidy/misc-assert-side-effect.cpp
67

Is it intentional that the check also warns on free functions even if they are marked as const, .e.g:

cat >test.cpp <<EOL

#include <cassert>

// only works for positive values and zero
template <typename T>
inline constexpr bool __attribute__((const))
isPowerOfTwo(T val) {
  return (val & (~val+1)) == val;
}

int main()
{
  assert(isPowerOfTwo(2 << 2));
}

EOL

Run:

$ clang-tidy -checks misc-assert-side-effect -config="{Checks: 'misc-assert-side-effect', CheckOptions: [{key: 'misc-assert-side-effect.CheckFunctionCalls', value: 1}]}" test.cpp -- -std=c++11
/tmp/test.cpp:14:3: warning: found assert() with side effect [misc-assert-side-effect]
  assert(isPowerOfTwo(2 << 2));
  ^
/usr/include/assert.h:89:4: note: expanded from macro 'assert'
  ((expr)                                                               \
   ^

Am i missing something obvious?

lebedev.ri added inline comments.Dec 13 2017, 5:47 AM
test/clang-tidy/misc-static-assert.cpp
71

Hmm. Are you sure about this?
I'm having false-positives (as far i'm concerned at least) due to this.

In particular with <sanitizer/asan_interface.h>. If the build is with asan,
then i can include the header and use e.g. __asan_region_is_poisoned()
But if it is not, i can't do that. So i need to wrap it into a macro, which expands
to __asan_region_is_poisoned() if the build is with asan, and to some constant
(e.g. 0) otherwise. And the check, naturally, does not like this.

As a minimal reproducer:

#ifndef MYDEFINE
#define mycheck(x) 0
#define notmycheck(x) 1
#else
int __mycheck(int x);
int __notmycheck(int x);
#define mycheck(x) __mycheck(x)
#define notmycheck(x) __notmycheck(x)
#endif

void f(int x) {
  assert(!mycheck(x));
  assert(!notmycheck(x));
  assert(mycheck(x));
  assert(notmycheck(x));
}

I'd expect the check to not warn regardless of MYDEFINE presence.