apelete added inline comments.
================
Comment at: lib/AST/ASTDiagnostic.cpp:1686
@@ -1685,3 +1685,3 @@
- if (Same) {
+ if (Same && FromTD) {
OS << "template " << FromTD->getNameAsString();
----------------
rtrieu wrote:
> dblaikie wrote:
> > Should this be a condition, or just an assertion?
> This should be an assertion. Same == true implies FromTD and ToTD are not
> null.
What do you mean by "This should be an assertion" ?
There's already an assertion on FromTD and ToTD values at the beginning of
PrintTemplateTemplate() so duplicating it here didn't make sense to me: should
I replace the if() with an assertion anyway ?
================
Comment at: lib/AST/ExprConstant.cpp:1992
@@ -1991,2 +1991,3 @@
int64_t Adjustment) {
+ assert(E && "expression to be evaluated must be not NULL");
CharUnits SizeOfPointee;
----------------
dblaikie wrote:
> Does the static analyzer assume any pointer parameter may be null? (I didn't
> think it did that - I thought it only assumed it could be null if there was a
> null test somewhere in the function?) That seems a bit too pessimistic in
> most codebases, especially LLVM - we pass a lot of non-null pointers around.
Not all pointer parameters trigger a warning during static analysis of Clang
codebase, but for the ones that do the reason isn't always clear to me (I'm
probably missing some background on static analysis here).
Let me know if you think the warning here should be ignored.
================
Comment at: lib/AST/NestedNameSpecifier.cpp:460
@@ +459,3 @@
+
+ assert(Buffer && "Buffer cannot be NULL");
+
----------------
dblaikie wrote:
> Again, is this assuming that the Buffer parameter may be null? That seems
> unlikely to be useful in the LLVM codebase where we have lots of guaranteed
> non-null pointers floating around (I'm surprised this wouldn't cause tons of
> analyzer warnings, so presumably it's something more subtle?)
Code path proposed by the analyzer is the following:
484. NestedNameSpecifierLocBuilder::
485. NestedNameSpecifierLocBuilder(const NestedNameSpecifierLocBuilder &Other)
486. : Representation(Other.Representation), Buffer(nullptr),
487. BufferSize(0), BufferCapacity(0)
488. {
...
500. Append(Other.Buffer, Other.Buffer + Other.BufferSize, Buffer, BufferSize,
501. BufferCapacity);
502. }
The constructor is called on line 484 and it initalizes Buffer to nullptr,
BufferSIze to 0 and BufferCapacity to 0 as shown above.
Then, after evaluating all branches in the constructor body to false, it calls
Append() with the parameters previously initialized:
441. void Append(char *Start, char *End, char *&Buffer, unsigned &BufferSize,
442 unsigned &BufferCapacity) {
443. if (Start == End)
444. return;
445.
446. if (BufferSize + (End - Start) > BufferCapacity) {
...
458. }
459.
460. memcpy(Buffer + BufferSize, Start, End - Start);
At this point, after evaluating the branches at lines 443 and 446 to false, the
analyzer concludes that 'Buffer + BufferSize' is a "Null pointer passed as an
argument to a 'nonnull' parameter", hence the assert I proposed just before
this line.
It turns out that this suggested codepath is indeed unlikely because either:
- Buffer parameter is null and so is BufferCapacity when Append() is called, in
which case branch at line 446 is unlikely to evaluate to false, or
- Buffer parameter is initialized to a non-null value in the constructor at
line 494 and is passed as a non-null parameter when Append() is called.
I chose however to trust the tool and prevent unforseen conditions to lead to
that path by asserting Buffer value.
Let me know if you think the warning here should be ignored.
http://reviews.llvm.org/D19084
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
