This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add the abseil-duration-comparison check
ClosedPublic

Authored by hwright on Nov 19 2018, 6:49 PM.

Details

Summary

This check finds instances where Duration values are being converted to a numeric value in a comparison expression, and suggests that the conversion happen on the other side of the expression to a Duration. See documentation for examples.

This also shuffles some code around so that the new check may perform in sone step simplifications also caught by other checks.

Diff Detail

Event Timeline

hwright created this revision.Nov 19 2018, 6:49 PM
JonasToth retitled this revision from Add the abseil-duration-comparison check to [clang-tidy] Add the abseil-duration-comparison check.Nov 20 2018, 1:37 AM
JonasToth added reviewers: alexfh, hokein.
JonasToth set the repository for this revision to rCTE Clang Tools Extra.
JonasToth added a project: Restricted Project.

Could you please upload the diff with full context? Especially the parts with refactoring are harder to judge if the code around is not visible. Thank you :)

clang-tidy/abseil/DurationComparisonCheck.cpp
26

I think this could be made a DenseMap with just the right amount of dense entries. 12 elements seems not too much to me.
Does the key-type need to be std::string, or could it be StringRef(or StringLiteral making everything constexpr if possible)?
Is there some strange stuff with dangling pointers or other issues going on?

69

The basis for this "we know" might change in the future if abs::Duration does things differently. Is adding stuff allowed in the absl:: space? (i am not fluent with the guarantees that it gives). You could maybe assert that the find is always correct, depending on absl guarantees.

JonasToth added inline comments.Nov 20 2018, 2:10 AM
clang-tidy/abseil/DurationComparisonCheck.cpp
51

This variable is a little hard to read. Could you make a little wrapper-struct instead of the tuple to make clear which element represents what?
Otherwise, why not std::pair?

  • same DenseMap argument
  • same StringRef/StringLiteral instead string consideration
79

Please no auto here

80

Please use const auto* to make it clear that this is a pointer

100

please no auto, you can ellide the braces

105

ellide braces

109

in LLVM the TODO does not contain a name for the author.

126

the list here is somewhat duplicated with the static members in the functions at the top. it would be best to merge them.
Not sure on how much constexpr is supported by the llvm-datastructures, but a constexpr DenseMap.keys() would be nice. Did you try something along this line?

152

Please follow the naming convention and don't use auto here.

clang-tidy/abseil/DurationFactoryFloatCheck.cpp
61

braces can be ellided here

clang-tidy/abseil/DurationRewriter.cpp
52

This is a implicit pointer-to-bool-conversion, isn't it? I think for readability purposes this should be made clear with a binary comparison.

62

const auto *

96

please no auto, braces

101

same

docs/clang-tidy/checks/abseil-duration-comparison.rst
11

'floating-pointer' -> 'floating-point'?

test/clang-tidy/abseil-duration-comparison.cpp
69

The test-cases miss macros and templates, please add reasonable cases for each of these categories.

90

What would happen for a type, that can implicitly convert to a duration or double/int.

struct MetaBenchmarkResults {
    int64_t RunID;
   absl::Duration D;

  operator absl::Duration() { return D; }
};

auto R = MetaBenchmarkResults { /* Foo */ };
bool WithinReason = Threshold < R;

What is the correct behaviour there? I think this should be diagnosed too.

162

could you please add a cases similiar to this:

if (some_condition && very_very_very_long_variable_name
     < SomeDuration) {
  // ...
}
JonasToth added inline comments.Nov 20 2018, 2:28 AM
test/clang-tidy/abseil-duration-comparison.cpp
90

Hmmmm, I fiddled around in godbolt, and i think there is nothing to trick around with conversions, as they wont compile.
If you find something along the lines, please tell me. But i guess this is a non-issue.

https://godbolt.org/z/gqhhHo

154

Please test the other casts you match on as well (especially c-style)

Eugene.Zelenko added inline comments.
docs/clang-tidy/checks/abseil-duration-comparison.rst
12

Please fix double space.

35

Please remove ..

hwright updated this revision to Diff 175379.Nov 26 2018, 6:39 PM
hwright marked 23 inline comments as done.

Sorry it's taken so long to get all the feedback addressed!

clang-tidy/abseil/DurationComparisonCheck.cpp
26

Conceptually, this could easily be constexpr, but my compiler doesn't seem to want to build something which is called static constexpr llvm::DenseMap<llvm::StringRef, DurationScale>. It's chief complaint is that such a type has a non-trivial destructor. Am I using this correctly?

51

std::pair works here.

I'll defer the DenseMap and StringRef/StringLiteral changes until we determine if they are actually possible.

69

absl:: could add things. In this case, I'm very confident they won't, but I've still added the assert.

109

I just removed the TODO (this isn't a required part of the check).

126

I haven't tried that exact formulation, but given the above issue with DenseMap, I'm not sure it will work. Happy to try once we get it ironed out.

Another thing I've thought about is factoring the functionDecl matcher into a separate function, because I expect it to be reused. I haven't been able to deduce what type that function would return.

JonasToth added inline comments.Nov 27 2018, 1:32 AM
clang-tidy/abseil/DurationComparisonCheck.cpp
26

I honestly never tried to make a constexpr DenseMap<> but it makes sense it is not possible, as DenseMap is involved with dynamic memory after all, which is not possible with constexpr (yet). So you were my test-bunny ;)

I did reread the Data-structures section in the LLVM manual, i totally forgot about StringMap that maps strings to values.
DenseMap is good when mapping small values to each other, as we do here (const char* -> int (whatever the enum deduces too)), which would fit.
StringMap does allocations for the strings, which we don't need.

constexpr aside my bet is that DenseMap fits this case the better, because we can lay every thing out and never touch it again. Maybe someone else has better arguments for the decision :)

Soooooo: could you please try static const llvm::DenseMap<llvm::StringRef, DurationScale>? :)
(should work in theory: https://godbolt.org/z/Qo7Nv4)

126

the type is probably clang::ast_matchers::internal::Matcher<FunctionDecl>. Not sure what the bind does with the type though.

If that is not the case you can make a nice error with int Bad = functionDecl(anything());, the type should be printed somewhere ;)
You could maybe create a lambda for that matcher or an AST_MATCHER(...) which some checks do as well. All variants are in use with clang-tidy.

clang-tidy/abseil/DurationRewriter.h
21

You could specify the underlying type to std::int8 directly, making the enum smaller and a more space-efficient DenseMap.

hwright updated this revision to Diff 175476.Nov 27 2018, 6:57 AM
hwright marked 10 inline comments as done.
hwright added inline comments.
clang-tidy/abseil/DurationComparisonCheck.cpp
26

static const llvm::DenseMap<llvm::StringRef, DurationScale> works here. :)

51

...but using DenseMap here doesn't work. From the mountains of compiler output I get when I try, it appears that DenseMap adds some constraints on the key type, and an enum class doesn't meet those constraints.

fwiw, the errors are mostly of this form:

/llvm/llvm/include/llvm/ADT/DenseMap.h:1277:45: error: ‘getEmptyKey’ is not a member of ‘llvm::DenseMapInfo<clang::tidy::abseil::DurationScale>’
     const KeyT Empty = KeyInfoT::getEmptyKey();
                        ~~~~~~~~~~~~~~~~~~~~~^~
/llvm/llvm/include/llvm/ADT/DenseMap.h:1278:53: error: ‘getTombstoneKey’ is not a member of ‘llvm::DenseMapInfo<clang::tidy::abseil::DurationScale>’
     const KeyT Tombstone = KeyInfoT::getTombstoneKey();
                            ~~~~~~~~~~~~~~~~~~~~~~~~~^~
/llvm/llvm/include/llvm/ADT/DenseMap.h:1280:44: error: ‘isEqual’ is not a member of ‘llvm::DenseMapInfo<clang::tidy::abseil::DurationScale>’
     while (Ptr != End && (KeyInfoT::isEqual(Ptr[-1].getFirst(), Empty) ||
                           ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
/llvm/llvm/include/llvm/ADT/DenseMap.h:1281:44: error: ‘isEqual’ is not a member of ‘llvm::DenseMapInfo<clang::tidy::abseil::DurationScale>’
                           KeyInfoT::isEqual(Ptr[-1].getFirst(), Tombstone)))
126

I was able to get both the factory matcher and the conversion matcher factored out. It's still a little messy, since the result has to be wrapped with a functionDecl at the call site, but that how the type system wanted it. (And it also allows for the bind call to work.)

sammccall added inline comments.
clang-tidy/abseil/DurationComparisonCheck.cpp
26

Sorry for the drive-by...
This has a non-trivial destructor, so violates https://llvm.org/docs/CodingStandards.html#do-not-use-static-constructors at least in spirit.

Best to leak it (auto &ScaleMap = *new const llvm::DenseMap<...>(...)) to avoid the destructor issues. The constructor issues are already handled by making it function-local.

aaron.ballman added inline comments.Nov 27 2018, 7:14 AM
clang-tidy/abseil/DurationComparisonCheck.cpp
26

I do not think this violates the coding standard -- that's talking mostly about global objects, not local objects, and is concerned about startup performance and initialization orders. I don't see any destructor ordering issues with this, so I do not think any of that applies here and leaking would be less than ideal.

sammccall added inline comments.Nov 27 2018, 7:24 AM
clang-tidy/abseil/DurationComparisonCheck.cpp
26

There are three main issues with global objects that the coding standard mentions:

  • performance when unused. Not relevant to function-local statics, only constructed when used
  • static initialization order fiasco (constructors). Not relevant to function-local statics, constructed in well-defined order
  • static initialization order fiasco (destructors). Just as relevant to function-local statics as to any other object!

That's why I say it violates the rule in spirit: the destructor is global. https://isocpp.org/wiki/faq/ctors#construct-on-first-use-v2

I don't see any destructor ordering issues with this

The reason these constructs are disallowed in coding guidelines is that humans can't see the problems, and they manifest in non-local ways :-(

leaking would be less than ideal

Why? The current code will (usually) destroy the object when the program exits. This is a waste of CPU, the OS will free the memory.

aaron.ballman added inline comments.Nov 27 2018, 7:30 AM
clang-tidy/abseil/DurationComparisonCheck.cpp
26

The reason these constructs are disallowed in coding guidelines is that humans can't see the problems, and they manifest in non-local ways :-(

Can you explain why you think this would have any non-local impact on destruction? The DenseMap is local to the function and a pointer to it never escapes (so nothing can rely on it), and the data contained are StringRefs wrapping string literals and integer values.

Why? The current code will (usually) destroy the object when the program exits. This is a waste of CPU, the OS will free the memory.

Static analysis tools will start barking about memory leaks which then adds noise to the output, hiding real issues.

sammccall added inline comments.Nov 27 2018, 7:58 AM
clang-tidy/abseil/DurationComparisonCheck.cpp
26

Can you explain why you think this would have any non-local impact on destruction?

I haven't analyzed this case in detail. A trivial example is running this check on a detached thread (sure, don't do that, but that's a non-local constraint...). It's possible there are no others here.

Static analysis tools will start barking about memory leaks which then adds noise to the output, hiding real issues.

This is a common pattern (from C++ FAQ) that tools are IME aware of. The object is still reachable so it's not truly a leak. (The heap checkers I know of are dynamic, but this is even easier to recognize statically).

(@hwright: sorry for the derail here, I know you're aware of this issue)

JonasToth added inline comments.Nov 27 2018, 8:20 AM
clang-tidy/abseil/DurationComparisonCheck.cpp
51

in order to use DenseMap you need to specialize the DenseMapInfo for types, that are not supported yet. More in the llvm manual and by examples.
Storing the integer is no issue, but the enum class makes its a type on its own, same with the pair.

I believe its fine to leave it an unordered_map, even though it might be slower on access. You can certainly do the extra-mile.

JonasToth added inline comments.Nov 27 2018, 8:32 AM
clang-tidy/abseil/DurationRewriter.h
63

I think you can even make this an AST_MATCHER(FunctionDecl, durationConversionFunction) { ... }, or was there an issue with it? (git grep -n AST_MATCHER in clang-tidy for other examples)
With this, the wrapping with functionDecl() should not be necessary.

JonasToth added inline comments.Nov 27 2018, 8:34 AM
clang-tidy/abseil/DurationRewriter.h
63

Nevermind, that was wrong. That would do functionDecl(durationConversionFunction()), sorry for noise.

aaron.ballman added inline comments.Nov 27 2018, 10:02 AM
clang-tidy/abseil/DurationComparisonCheck.cpp
26

I guess I still read the coding standard differently. While it's certainly possible to have a finalization order fiasco with local statics, I believe you have to work considerably harder to craft one. I was doing some code archaeology on the words in the coding standard, and I don't think this was the scenario we had in mind when crafting that rule, but these words have been around for quite some time and I may have missed contextual discussion from the mailing lists.

It might make sense to propose a patch that adds some clarity to the coding standard in this area. We use static locals like this in numerous places (IdentifierNamingCheck.cpp, TypePromotionInMathFnCheck.cpp, TypeMismatchCheck.cpp, etc) and it would be good to nail down what we want to have happen in that case.

As for this code review, I still think using a heap allocation is not a good approach. If there is a serious concern about finalization ordering, we can use a ManagedStatic instead.

hwright marked 12 inline comments as done.Nov 27 2018, 10:23 AM
hwright added inline comments.
clang-tidy/abseil/DurationComparisonCheck.cpp
26

From all this discussion, it sounds like leaving the heap allocation as-is is what we want here.

Though I also understand an am sad about the potential for finalization ordering, and I'm doubly sad that C++ doesn't give us a good way to create a static map like this which doesn't require any finalization.

51

I'll just leave this as-in, since it looks like DenseMapInfo wants unused sentinel values, which I'm loathe to add to the enum.

hwright marked 2 inline comments as done.Nov 28 2018, 9:35 AM

Anything else for me here?

LG from my side, only the style nits left.
other reviewers are invited to take a look too :)

clang-tidy/abseil/DurationComparisonCheck.cpp
23

Please use triple / for the function comment, for doxygen and consistency with documentation

70

non-existence? Not sure about english, but i thought english does it that way

clang-tidy/abseil/DurationComparisonCheck.h
21

Missing /

clang-tidy/abseil/DurationDivisionCheck.h
23 ↗(On Diff #175476)

I think that blank line could be removed, and it seems the comment is not ///, could you take a look at it too?
Touching this file is probably better to do in another patch anyway.

clang-tidy/abseil/DurationFactoryFloatCheck.cpp
61

The diagnostic is not printed if for some reason the fixit was not creatable. I think that the warning could still be emitted (auto Diag = diag(...); if (Fix) Diag << Fixit::-...)

clang-tidy/abseil/DurationRewriter.cpp
20

Comment

76

you can ellide the braces

86

you can ellide the braces here

clang-tidy/abseil/DurationRewriter.h
22

Same comment things in this file (///)

57

Double space

hwright updated this revision to Diff 175735.Nov 28 2018, 11:40 AM
hwright marked 13 inline comments as done.
hwright added inline comments.
clang-tidy/abseil/DurationComparisonCheck.cpp
70
clang-tidy/abseil/DurationDivisionCheck.h
23 ↗(On Diff #175476)

Agreed. I think this snuck into the patch; I'll remove it.

(It would be good to just clang-format everything in this directory in a separate patch.)

The comment issue with /// seems to be a common problem; is clang-tidy/add_new_check.py not generating correct code?

clang-tidy/abseil/DurationFactoryFloatCheck.cpp
61

I'm not sure under which conditions you'd expect this to be an issue. Could you give me an example?

My expectation is that if we don't get a value back in SimpleArg, we don't have anything to change, so there wouldn't be a warning to emit.

JonasToth added inline comments.Nov 28 2018, 12:03 PM
clang-tidy/abseil/DurationDivisionCheck.h
23 ↗(On Diff #175476)

add_new_check does ///, maybe IDE settings or so removed these? Maybe someone created everything manually, dunno.

Doing the clang-format is ok, doesn't need review either (but plz run the test before committing to master).

clang-tidy/abseil/DurationFactoryFloatCheck.cpp
61

I don't expect this to fail, failure there would mean a bug i guess. Having bugs is somewhat expected :)
And that would be our way to find the bug, because some user reports that there is no transformation done, that is my motivation behind that.

The warning itself should be correct, otherwise the matcher does not work, right? This would just be precaution.

hwright marked 4 inline comments as done.Nov 28 2018, 1:38 PM
hwright added inline comments.
clang-tidy/abseil/DurationDivisionCheck.h
23 ↗(On Diff #175476)

I don't think I yet have the commit bit...so somebody else wouldn't need to directly commit. :)

clang-tidy/abseil/DurationFactoryFloatCheck.cpp
61

I guess what I'm saying is that I'm not sure what kind of warning we'd give if we weren't able to offer a fix. The optionality here means that we found a result which was already as simple as we can make it, so there's no reason to bother the user.

JonasToth added inline comments.Nov 28 2018, 1:55 PM
clang-tidy/abseil/DurationFactoryFloatCheck.cpp
61

Lets say the result comes out of arithmetic and then there is a comparison (would be good test btw :)), the warning is still valueable, even if the transformation fails for some reason. The transformation could fail as well, because macros interfere or so. Past experience tells me, there are enough language constructs to break everything in weird combinations :)

hwright marked 4 inline comments as done.Nov 29 2018, 4:00 AM
hwright added inline comments.
clang-tidy/abseil/DurationFactoryFloatCheck.cpp
61

I can buy that, but I'm still having trouble seeing the specifics. Do you have a concrete example?

hwright marked an inline comment as done.Nov 29 2018, 4:53 PM

Ping.

hwright updated this revision to Diff 176103.Nov 30 2018, 5:48 AM

Slightly simplify the fixit text

JonasToth accepted this revision.Nov 30 2018, 8:28 AM

Only the test and your opinion on the Optional thing, If you want to keep it that way its fine.

LGTM afterwards :)

clang-tidy/abseil/DurationFactoryFloatCheck.cpp
61

No i can't think of one right now. Its more a general argument, that the code-layout logically allows that only the transformation fails, even if the detection succeeds. No strong opinion, but preference :)

test/clang-tidy/abseil-duration-comparison.cpp
149

please add a test where the int part is result of a complex arithmetic expression.

This revision is now accepted and ready to land.Nov 30 2018, 8:28 AM
hwright updated this revision to Diff 176156.Nov 30 2018, 10:03 AM
hwright marked 2 inline comments as done.

Add additional test

@JonasToth reminder that you (or somebody else) will need to commit this for me.

Oh, and thanks for taking the time to review this. :)

Oh, and thanks for taking the time to review this. :)

No problem!
I will commit on Monday evening (Europe evening) if there are no further comments from other reviewers.
Thank you for the patch!

This revision was automatically updated to reflect the committed changes.

I had to revert and recommitted in rCTE348169. std::unordered_map<enum class Something, ...> does not work, as std::hash is not specialized for it. This behaviour seems to work for some compilers, but some not. I had to google myself a bit for the best solution, but now I specialized the hash-function to std::hash<std::int8>() in unordered_set which worked for me locally and was suggested. Lets see ;)

I had to revert and recommitted in rCTE348169. std::unordered_map<enum class Something, ...> does not work, as std::hash is not specialized for it. This behaviour seems to work for some compilers, but some not. I had to google myself a bit for the best solution, but now I specialized the hash-function to std::hash<std::int8>() in unordered_set which worked for me locally and was suggested. Lets see ;)

int8 ? Did you mean int8_t or am I missing somthing ?

int8 ? Did you mean int8_t or am I missing somthing ?

Your right, but the solution I wrote did actually not work anyway.. I just specialized std::hash<> now.