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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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? | |
218 | If you want a killer doc, use examples. Not saying this is not good enough, but it wouldn't hurt. | |
220–222 | 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. | |
262 | Nice. | |
1031 | seems like this could use clang-format-diff.py. |
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp | ||
---|---|---|
83 | 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. | |
218 | 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. | |
271 | This new private section is the new added code. | |
275 | The "@brief" is probably not necessary and not used often, I plan to remove it. | |
893 | This code is got from RangeConstraint::applyAsOutOfRange. | |
910 | This code is got from RangeConstraint::applyWithinRange. | |
1060 | This function is just moved here from the previous (inline) location. | |
1143 | 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. | |
271 | 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. |
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp | ||
---|---|---|
271 | 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. |
clang-format not found in user’s local PATH; not linting file.