Page MenuHomePhabricator

[analyzer][NFC] Consolidate the inner representation of CallDescriptions
ClosedPublic

Authored by steakhal on Wed, Nov 10, 10:46 AM.

Details

Summary

CallDescriptions have a RequiredArgs and RequiredParams members,
but they are of different types, unsigned and size_t respectively.
In the patch I use only unsigned for both, that should be large enough
anyway.

In the patch, I also avoid the use of the smart less-than operator.

template <typename T>
constexpr bool operator<=(const Optional<T> &X, const T &Y);

Which would check if the optional has a value and compare the data
only after. I found it surprising, thus I think we are better off
without it.

Diff Detail

Event Timeline

steakhal created this revision.Wed, Nov 10, 10:46 AM
xazax.hun accepted this revision.Fri, Nov 12, 9:29 AM

Nice cleanup!

This revision is now accepted and ready to land.Fri, Nov 12, 9:29 AM

While you have a large stack, some of these changes should be relatively independent. I'd recommend committing those regardless of the status of the rest of the stack.

While you have a large stack, some of these changes should be relatively independent. I'd recommend committing those regardless of the status of the rest of the stack.

Using linear history is way more convenient for me.
I will split the stack if we don't reach a consensus on the stack though.

In the patch, I also avoid the use of the smart less-than operator.

It's not obvious for me how you avoid it. Could you please elaborate?

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
48–50

What about using a common type alias for these two?

In the patch, I also avoid the use of the smart less-than operator.

It's not obvious for me how you avoid it. Could you please elaborate?

You can see in the diff that in some cases I'm using the dereference operator of the Optional in the new code, whereas previously we did not.
It was because the operator== and operator<= did the nullness check of the Optional within their operator implementation.
As you can see, one does not regularly see such a thing, so we are better off with that IMO.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
48–50

We could have some alias type, but I would rather choose MaybeCount or MaybeInt.
Which would you prefer?

martong accepted this revision.Fri, Nov 19, 1:44 AM

In the patch, I also avoid the use of the smart less-than operator.

It's not obvious for me how you avoid it. Could you please elaborate?

You can see in the diff that in some cases I'm using the dereference operator of the Optional in the new code, whereas previously we did not.
It was because the operator== and operator<= did the nullness check of the Optional within their operator implementation.
As you can see, one does not regularly see such a thing, so we are better off with that IMO.

Okay, thanks, makes sense.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
48–50

I prefer MaybeCount, thanks for asking.

steakhal updated this revision to Diff 388433.Fri, Nov 19, 2:12 AM
steakhal marked 2 inline comments as done.
steakhal edited the summary of this revision. (Show Details)
  • introduce the MaybeUInt type alias
steakhal added inline comments.Fri, Nov 19, 2:21 AM
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
48–50

What about MaybeUInt? On second thought, MaybeCount might be even better.

This revision was landed with ongoing or failed builds.Fri, Nov 19, 9:33 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFri, Nov 19, 9:33 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript