In D53024#1258976, @Szelethus wrote:

Also, a lot of items on this list is outdated, but I joined the project relatively recently, so I'm not sure what's the state on all of them.

Greate idea! If we can enrich this list, it will not only help the reviewer, but also help beginners, like me, avoid some pitfalls when developing a new checker.

Thank you for digging in to delete that meaningless constructor, NoQ! And sorry for my technology debt : ).

In D48027#1209844, @NoQ wrote:

So i believe that one of the important remaining problems with CallDescription is to teach it to discriminate between global functions and methods. We can do it in a number of ways:

1. Make a special sub-class for CallDescription depending on the sort of the function (too verbose),
2. Tag all items in the list as "record" vs. "namespace" (too many tags),
3. Dunno, tons of other boring and verbose approaches,
4. Change our PreCall/PostCall callbacks to callback templates that let allow the user subscribe to specific sub-classes of CallEvent similarly to how he can subscribe to different kinds of statements in PreStmt<Class>/PostStmt<Class>, and then the user doesn't need to write any code to check it dynamically.

In D48027#1207645, @xazax.hun wrote:

Sorry for the delay, I think this is OK to commit.
As a possible improvement, I can imagine making it slightly stricter, e.g. you could only skip QualifiedNames for inline namespaces and the beginning. Maybe add support for staring with "" or :: to even disable skipping namespaces at the beginning?
But these are just nice to have features, I am perfectly ok with not having them or doing it in a followup patch.

@xazax.hun What do you think?

In D48027#1201248, @xazax.hun wrote:

Generally looks good, I only wonder if this works well with inline namespaces. Could you test? Probably it will.

• rebase
• Since we have enhanced the ability of CallDescription, remove the helper method isCalledOnStringObject().

kindly ping!

@xazax.hun Thanks for your tips! After some investigation, MatchFinder::match just traverse one ASTNode, that means match(namedDecl(HasNameMatcher())) and match(namedDecl(matchesName())) both not traverse children. So there are three ways to match the specified AST node.

Thanks for your review, NoQ!

kindly ping!

Sorry for the long long delay, I was on the Dragon Boat Festival a few days ago.

• Use hasName matcher to match the qualified name.

In D48027#1128301, @xazax.hun wrote:

Having C++ support would be awesome!
Thanks for working on this!

While I do agree matching is not trivial with qualified names, this problem is already solved with AST matchers.

I wonder if using matchers or reusing the HasNameMatcher class would make this easier. That code path is already well tested and optimized.

Remove useless header files for testing.

The implementation is not complicated, the difficulty is that there is no good way to get the qualified name without template arguments. For std::basic_string::c_str(), its qualified name may be std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::c_str, it is almost impossible for users to provide such a name. So one possible implementation is to use std, basic_string and c_str to match in the std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::c_str sequentially.

In D47973#1127309, @vsk wrote:

Thanks for the patch. The StringRef change itself looks fine, but please make sure to update the callsites in-tree (e.g in llvm, clang, lldb etc).

LGTM, @NoQ May have further feedback. Thanks!

I didn't test the code, but the code seems correct. Thanks!

In D47406#1120360, @zturner wrote:

rsplit(char) already exists. That said, another way to reuse code would be
to have rsplit(char) call the StringRef version

In D47406#1120355, @vsk wrote:

Hi @MTC, thanks for the patch.

With this patch, the logic for split would be duplicated 3 times now in StringRef.h. I think it'd be nice to reuse some code by moving the core logic into a private method of StringRef, say, splitAtIndex(size_t). Then, each (r)split(char | StringRef) can use the common helper.

Relatedly, an rsplit(char) overload seems to be missing here.

This update consists of two parts

• Make rsplit(char) reuse rsplit(StringRef).
• Make split(char) reuse split(StringRef).

ping.

• According to NoQ's suggestion, use assumeZero() instead of isZeroConstant() to determine whether the value is 0.
• Add test memset26_upper_UCHAR_MAX() and memset27_symbol()
• Since memset( void *dest, int ch, size_t count) will converts the value ch to unsigned char, we call evalCast() accordingly.

ping.

• Since there is no perfect way to handle the default binding of non-zero character, remove the default binding of non-zero character. Use bindDefaulrZero() instead of overwriteRegion() to bind the zero character.
• Reuse assume() instead of isZeroConstant() to determine whether it is zero character. The purpose of this is to be able to set the string length when dealing with non-zero symbol character.

In D44934#1088771, @NoQ wrote:

Hmm, ok, it seems that i've just changed the API in D46368, and i should have thought about this use case. Well, at least i have some background understanding of these problems now. Sorry for not keeping eye on this problem.

In D44934#1051002, @NoQ wrote:

Why do you need separate code for null and non-null character? The function's semantics doesn't seem to care.

I guess i can answer myself here:

int32_t x;
memset(&x, 1, sizeof(int32_t));
clang_analyzer_eval(x == 0x1010101); // should be TRUE

I really doubt that we support this case.

So, yeah, zero character is indeed special.

Thank you, Artem! I did not consider this common situation. This patch does not really support this situation, in this patch the value of x will be 1, it's not correct!

• fix typos
• code refactoring, add auxiliary method memsetAux()
• according to a.sidorin's suggestions, remove the useless state splitting.
• make StoreManager::overwriteRegion() pure virtual

Sorry for the long delay, I have just finished my holiday.

ping^2

Since BugReport::addVisitor() has checks for the null Visitor, remove the checks before BugReport->addVisitor().

In D45682#1074407, @george.karpenkov wrote:

I'm new to the taint visitor, but I am quite confused by your change description.

and many checkers rely on it

How can other checkers rely on it if it's private to the taint checker?

Thanks for your review, george! TaintBugVisitor is an utility to add extra information to illustrate where the taint information originated from. There are several checkers use taint information, e.g. ArrayBoundCheckerV2.cpp, in some cases it will report a warning, like warning: Out of bound memory access (index is tainted). If TaintBugVisitor moves to BugReporterVisitors.h, ArrayBoundCheckerV2 can add extra notes like Taint originated here to the report by adding TaintBugVisitor.

Test files for initialization missing? : )

In D45532#1069683, @whisperity wrote:

There is something that came up in my mind:

Consider a construct like this:

class A
{
  A()
  {
    memset(X, 0, 10 * sizeof(int));
  }

  int X[10];
};

I think it's worthy for a test case if this X is considered unitialised at the end of the constructor or not. (And if not, a ticket, or a fix for SA in a subpatch.)

In D45491#1067852, @NoQ wrote:

Yeah, i think this makes sense, thanks! It feels a bit weird that we have to add it as an exception - i wonder if there are other exceptions that we need to make. Widening over the stack memory space should be a whitelist, not a blacklist, because we can easily enumerate all stack variables and see which of them can be modified at all from the loop. But until we have that, this looks like a reasonable workaround.

• Move the CXXThisRegion's check to LoopWidening.cpp
• Use isa<CXXThisRegion>(R) instead of CXXThisRegion::classof(R).

In D45491#1063364, @george.karpenkov wrote:

@MTC what happens for

this.j = 0;
for (int i=0; i<100; i++)
   this.j++;

?

Kindly ping!

Thank you for your reminding, I overlooked this point. However for non-concrete character, the symbol value, if we just invalidate the region, the constraint information of the non-concrete character will be lost. Do we need to consider this?

Fix typo, unsinged -> unsigned

According to @NoQ's suggestion, remove the duplicated code.

Thanks for your review, NoQ!

Thank you for taking the time to pay attention to this problem, @NoQ. The reason for the test regression is that CheckBufferAccess() does not guarantee that CheckNonNull() must be called for the second buffer, see https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Checkers/CStringChecker.cpp#L385.

Add the comments as suggested by @szepet .

In D44557#1042357, @NoQ wrote:

Sorry, one moment, i'm seeing a few regressions after the previous refactoring but i didn't look at them closely yet to provide a reproducer. I'll get back to this.

Just in case: we indeed do not guarantee that SymbolConjured corresponds to a statement; it is, however, not intended, but rather a bug.

Thank you for your explanation and the reasonable example, NoQ.

One small nit for future debugging people: Could you insert a comment line in the test case where you explain what is this all about? E.g what you just have written in the description: "invalidateRegions() will construct the SymbolConjured with null Stmt" or something like this.

Remove the default configuration -analyzer-store=region in the test file.

@NoQ, Very sorry, I've forgotten about this patch, it has now been updated.

Update the taint-generic.c to test both stdin declaration variants.

Thank you for your review, @NoQ!

• If the operand of the ++ operator is of type _Bool, also set to true.
• Add test file _Bool-increment-decement.c.

In D43741#1024949, @alexshap wrote:

i see, but just in case - what about the decrement operator ?

@NoQ Sorry to bother you again. It seems that this patch is useless to analyzer temporarily, if you think so, I will abandon it : ).

rebase

In D42785#995211, @NoQ wrote:

Ew. So it means that checker transitions are currently discarded. Great catch. I guess we don't use this functionality yet, so we can't test it, but the fix should definitely go in.

You are right, that's why I don't know how to add test for this change.

In D42300#982187, @NoQ wrote:

My intuition suggests that this checker shouldn't be path-sensitive; our path-sensitive analysis does very little to help you with this particular checker, and you might end up with a much easier and more reliable checker if you turn it into a simple AST visitor or an AST matcher. Just a heads up.

• Use C++11 range-based for loop to traverse ExplodedNodeSet.
• Define the macro offsetof in system-header-simulator.h.