This is an archive of the discontinued LLVM Phabricator instance.

[clang][analyzer][NFC] Refactor code of StdLibraryFunctionsChecker.
ClosedPublic

Authored by balazske on Feb 10 2023, 9:01 AM.

Details

Summary

The code was difficult to maintain (big internal class definitions
with long inline functions, other functions of the same class at
different location far away, irregular ordering of classes and
function definitions). It is now improved to some extent.
New functions are added to RangeConstraint to remove code repetition,
these are useful for planned new features too.
Comments are improved.

Diff Detail

Event Timeline

balazske created this revision.Feb 10 2023, 9:01 AM
Herald added a reviewer: NoQ. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
balazske requested review of this revision.Feb 10 2023, 9:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2023, 9:01 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
balazske updated this revision to Diff 496882.Feb 13 2023, 2:11 AM

small problem fix

balazske updated this revision to Diff 496923.Feb 13 2023, 3:53 AM

removed "IsLast"

Ugh, I admit, its a little hard to follow what happened here. You moved a lot of code around (I agree with that!), but also changed code as well. Can you just summarize what is NOT just moved code and needs a more thorough look?

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
124–125

Is this what you meant?

222

If you want a killer doc, use examples. Not saying this is not good enough, but it wouldn't hurt.

232–234

I'd place parts of this comments above the enum values declaration (now on line 72), but even that small comment that is already there looks good enough. After that, maybe there is not much to note here.

267

Nice.

1046

seems like this could use clang-format-diff.py.

balazske added inline comments.Feb 22 2023, 8:25 AM
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
89

This enum is moved here from other location, and negateKind is added.

124–125

Probably this is better:

// Mind that a pointer to a new value constraint can be created by negating an existing
// constraint.

The important thing is that one reason for the shared pointer is the negate function that returns a pointer.

222

The "range" is not the best term for this object in the documentation. A "range" is often used when a single interval is meant, but here it often means a set of ranges. I plan to improve this.

276

This new private section is the new added code.

280

The "@brief" is probably not necessary and not used often, I plan to remove it.

912–983

This code is got from RangeConstraint::applyAsOutOfRange.
This becomes "within" because in the apply as out of range case all ranges are cut out, that is really a loop over ("within") the ranges.

929

This code is got from RangeConstraint::applyWithinRange.
That was implemented by removal of all out-of-range intervals. The code here is a general version that calls a lambda instead of the actual assume calls, so it becomes reusable for other purposes when a loop over all (out-of) intervals is used.

1075

This function is just moved here from the previous (inline) location.

1158

The new argument in reportBug is not used but is used in the next patch.

A high level comment before getting into (even more) nitty gritty stuff. But shut me down if I misunderstood whats happening.

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
124–125

Can be, or will be? But otherwise, sure.

276

While generalizing code is great, whenever its done by introducing function arguments / lambdas, the code becomes harder to understand. This is fine, as long as this complexity is justified, but I really needed to see what happened in the followup patch to see whats the gain behind this.

The gain is that you can capture a stream and construct a helpful message as the range is applied.

Question: couldn't you just expose a lambda for the specific purpose for string smithing, and only that? Seems like all lambdas contain kind of the same thing: a call to ConstraintManager::assumeInclusiveRange.

Maybe this design (for the above reasons) is worth documenting.

balazske added inline comments.Mar 2 2023, 6:51 AM
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
276

It looks not easy to change the design, the lambda is not only used for string construction and the way the state is applied (only check the state or change it too) is different. Probably it is possible but with more additional special purpose function parameters that add again complexity. Otherwise the current way looks not difficult, just a function that is called on all ranges or out-of-ranges, like an iteration. It can be possible to make a function that instead of calling lambda returns a list of ranges for the within-range or out-of-range case, and then make an iteration over these lists, but this is really only more code, and the returned lists are used only for these iterations.

balazske updated this revision to Diff 502143.Mar 3 2023, 8:40 AM

changed comments, small variable rename

balazske updated this revision to Diff 502153.Mar 3 2023, 9:06 AM

rebase, reformatted code

balazske marked 5 inline comments as done.Mar 3 2023, 9:08 AM
This revision is now accepted and ready to land.Mar 9 2023, 1:42 AM