a.sidorin added a comment.
Looks good now, thanks!
Repository:
rL LLVM
https://reviews.llvm.org/D27180
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
a.sidorin added a comment.
Looks good for me, but I'm not a reviewer. Thank you Devin!
Comment at: test/Driver/analyzer-target-enabled-checkers.cpp:7
// CHECK-DARWIN: "-analyzer-checker=core"
+// CHECK-DARWIN-SAME: "-analyzer-checker=apiModeling"
// CHECK-DARWIN-SAME: "-analy
a.sidorin added a comment.
Looks useful and mostly good. A small advice is in inline comments.
Comment at: lib/StaticAnalyzer/Core/BugReporter.cpp:3294
+ // Find the node's current statement in the CFG.
+ // FIXME: CFG lookup should be made easier.
+ const CFG &Cfg = N->getC
a.sidorin added inline comments.
Comment at: lib/StaticAnalyzer/Core/BugReporter.cpp:3363
+// we're post-dominated by.
+// FIXME: This is far from enough to handle the incomplete analysis case.
+// We may be post-dominated in subsequent blocks, or even
---
a.sidorin added a comment.
Kareem, I have re-checked it and I cannot see the failure. But I'm not going to
commit it until NY holidays end (and, anyway, I will not commit it if somebody
has failing tests). Could you describe your configuration?
https://reviews.llvm.org/D26753
__
a.sidorin added a comment.
Hello Daniel,
Your patch looks mostly good to me. I have only minor naming comments.
Comment at: lib/StaticAnalyzer/Core/CheckerHelpers.cpp:99
+bool clang::ento::isExprResultConformsComparisonRule(CheckerContext &C,
+
a.sidorin added inline comments.
Comment at: lib/StaticAnalyzer/Core/CheckerHelpers.cpp:99
+bool clang::ento::isExprResultConformsComparisonRule(CheckerContext &C,
+ BinaryOperatorKind BOK,
+
a.sidorin added inline comments.
Comment at: lib/AST/ASTImporter.cpp:4305
+ DeclarationNameInfo NameInfo(Name,
Importer.Import(D->getNameInfo().getLoc()));
+ ImportDeclarationNameLoc(D->getNameInfo(), NameInfo);
+
spyffe wrote:
> I've seen this pattern before,
a.sidorin accepted this revision.
a.sidorin added a comment.
Looks excellent now. Thank you Sean!
Repository:
rL LLVM
https://reviews.llvm.org/D30435
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/li
a.sidorin added a comment.
Amazing work, Dominic. That's what I wanted to test for long time. But,
personally, I'm not happy with massive changes in tests.
1. I don't think that we need to change run line for tests if they pass with
both managers. These changes are pretty noisy,
2. If Z3 is opt
a.sidorin added a comment.
I got it. I have hard-coded paths in CHECK-lines so these tests are passed on
my machine but not on other. Thank you Kareem!
https://reviews.llvm.org/D26753
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://l
This revision was automatically updated to reflect the committed changes.
Closed by commit rL292779: ASTImporter: quick test fix (authored by a.sidorin).
Changed prior to commit:
https://reviews.llvm.org/D26753?vs=79054&id=85332#toc
Repository:
rL LLVM
https://reviews.llvm.org/D26753
Files:
a.sidorin added a comment.
Main revisions: https://reviews.llvm.org/rL292776,
https://reviews.llvm.org/rL292778. Sorry for not mentioning them in
Differential Revision.
Repository:
rL LLVM
https://reviews.llvm.org/D26753
___
cfe-commits mailing
a.sidorin added a comment.
This should be fixed in r269693.
https://reviews.llvm.org/D6549
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
a.sidorin added a comment.
Hi Gabor. One of the bugs fixed in this patch is still present in master, other
two are already fixed.
Comment at: lib/AST/ASTImporter.cpp:2749
// Create the imported function.
+ SourceLocation StartLoc = Importer.Import(D->getInnerLocStart());
a.sidorin added a comment.
Hi Artem,
Thank you for adding me, I missed this patch. I have few comments below. If you
(and Vlad) can wait for two or three days, I will re-check the place I'm
worrying about and post the results.
Comment at: lib/StaticAnalyzer/Checkers/GenericT
a.sidorin added a comment.
Oh, I see now. I was wrongly thinking that these patches do the same thing and
we're duplicating the work. Thank you very much for your explanation, Devin.
Repository:
rL LLVM
https://reviews.llvm.org/D16403
___
cfe-co
a.sidorin accepted this revision.
a.sidorin added a comment.
This revision is now accepted and ready to land.
Looks good.
https://reviews.llvm.org/D35674
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/l
a.sidorin accepted this revision.
a.sidorin added a comment.
This revision is now accepted and ready to land.
Just a minor nit.
Comment at: lib/StaticAnalyzer/Core/BugReporter.cpp:3332
+
+if (Visited.count(Blk)) {
+ // We've encountered a loop. We won't see anything ne
a.sidorin added a comment.
Hello Adam,
This looks like a nice improvement. I have some remarks inline.
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:382
}
+ } else if (!isa(&Call)) {
+// The main purpose of iterators is to abstract away from different
--
a.sidorin added a comment.
Hello Adam. I have a nit inline.
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1256
+if (Cond(Reg.second)) {
+ State = setIteratorPosition(State, Reg.first, Proc(Reg.second));
+}
Updating ProgramState is usu
a.sidorin added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1256
+if (Cond(Reg.second)) {
+ State = setIteratorPosition(State, Reg.first, Proc(Reg.second));
+}
baloghadamsoftware wrote:
> a.sidorin wrote:
> > Updating
a.sidorin added a comment.
Hello Henry,
The patch looks reasonable. I think it can be landed after comments are
resolved.
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1497
+ ExplodedNodeSet PostVisit;
+ for (ExplodedNodeSet::iterator i = PreVisit.begin(), e =
a.sidorin created this revision.
a.sidorin added reviewers: xazax.hun, szepet, jingham.
a.sidorin added a project: clang.
Herald added subscribers: rnkovacs, kristof.beyls, aemerson.
Also, a number of style and bug fixes was done:
- ASTImporterTest: added sanity check for source node
- ExternalAS
a.sidorin added a comment.
Hello Peter,
Thank you for the patch! It is almost LGTM, just a few minor questions inline.
Am I understand correctly that it is partially based on
https://github.com/haoNoQ/clang/blob/summary-ipa-draft/lib/AST/ASTImporter.cpp?
Also, FYI: you can use ASTMerge and clang
a.sidorin added a comment.
> I do not see a test for the following changes:
>
> - ASTImporter: don't add templated declarations into DeclContext
It's in ASTImporterTest. It checks that the templated decl cannot be found in
the enclosing TU.
> - ASTImporter: proper set ParmVarDecls for imported
a.sidorin added inline comments.
Comment at: lib/AST/ASTImporter.cpp:6207
+TypeSourceInfo *TSI = Importer.Import(E->getTypeOperandSourceInfo());
+if (!TSI && E->getTypeOperandSourceInfo())
+ return nullptr;
As I see from usage of `getTypeOperandSourc
a.sidorin updated this revision to Diff 131279.
a.sidorin added a comment.
Addressed review comments
Repository:
rC Clang
https://reviews.llvm.org/D42301
Files:
lib/AST/ASTImporter.cpp
lib/AST/ASTStructuralEquivalence.cpp
lib/AST/ExternalASTMerger.cpp
test/ASTMerge/class-template/Inp
a.sidorin added a comment.
In https://reviews.llvm.org/D42301#984695, @xazax.hun wrote:
> High level note: `clang::TypeAliasDecl` has the same issue as
> `CXXRecordDecl`, so templated versions should not be added to the
> DeclContext. Could you add that just for the sake of completeness?
I'd
a.sidorin accepted this revision.
a.sidorin added a comment.
This revision is now accepted and ready to land.
LGTM, thank you!
https://reviews.llvm.org/D42335
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mail
a.sidorin added a comment.
Thank you, Gabor. I'll change cast<> to cast_or_null<> to enable the assertion
below.
Repository:
rC Clang
https://reviews.llvm.org/D42301
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/c
This revision was automatically updated to reflect the committed changes.
Closed by commit rC323519: [ASTImporter] Support LambdaExprs and improve
template support (authored by a.sidorin, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D42301?vs=131279&id=131568#toc
Repositor
a.sidorin added a comment.
Hello David,
Do you have any results of this checker on the real code? If yes, could you
please share them?
There are also some inline comments regarding implementation.
Comment at: lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp:42
+
+void Mma
a.sidorin added a comment.
More debug info is always good.
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:400
+Out << '(' << I.second << ',' << I.first << ") ";
+I.first->printPretty(Out, nullptr, PrintingPolicy(LO));
+Out << NL;
a.sidorin added inline comments.
Herald added subscribers: llvm-commits, rnkovacs.
Comment at: cfe/trunk/unittests/AST/ASTImporterTest.cpp:100
+ // This traverses the AST to catch certain bugs like poorly or not
+ // implemented subtrees.
I just saw this chan
a.sidorin added a comment.
Hi David! The patch looks almost OK.
Comment at: lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp:65
+ *BT, "Both PROT_WRITE and PROT_EXEC flags are set. This can "
+ "lead to exploitable memory regions, which could be overw
a.sidorin added a comment.
Sorry for the delay, I have commented inline.
For me, this patch looks like a strict improvement. I think, after some clean
up this can be accepted.
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:442
+
+const RecordDecl *RD = RT-
a.sidorin added a comment.
This patch lacks tests. If you add at least minimal test case (I'm not familiar
with ObjC and its front-end, unfortunately), I will no have any concerns. Also
adding Sean.
https://reviews.llvm.org/D6550
___
cfe-commits m
a.sidorin added a comment.
Hello Zoltan. Sorry, I'm a bit busy now. Here are my thoughts about the design.
1. I think we should not add new warnings to GenericTaintChecker. Instead, it
is better to move warnings out of it. After that it will become just a plugin
used by other checkers. Such sp
a.sidorin added a comment.
Anna, I will commit. Thank you!
https://reviews.llvm.org/D29643
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL295644: [analyzer] Do not duplicate call graph nodes for
functions that have definition… (authored by a.sidorin).
Changed prior to commit:
https://reviews.llvm.org/D29643?vs=87388&id=89089#toc
Reposito
a.sidorin added a comment.
Hi Artem! Thank you for this patch. It looks very promising, but I have some
questions and remarks.
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:187
const Expr *Result) {
- SVal V = State->getSVal(Ex,
a.sidorin added a comment.
Looks like a right fix.
Comment at: lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp:93
// Warn when there is widening cast.
unsigned ToWidth = Ctx.getTypeInfo(ToPointeeTy).Width;
NoQ wrote:
> I think we should move the
a.sidorin created this revision.
ExprEngine assumes that OpenMP statements should never appear in CFG. However,
current CFG doesn't know anything about OpenMP and passes such statements as
CFG nodes causing "UNREACHABLE executed!" crashes. Since I have no ideas on
OpenMP implementation in ExprE
a.sidorin added a comment.
`git blame` shows that OMP* statements were added to the switch block by
different authors while OpenMP support in clang was implemented. Looks like
they were put to "Should not appear" section instead of "Unsupported" by
mistake.
https://reviews.llvm.org/D30565
This revision was automatically updated to reflect the committed changes.
Closed by commit rL296884: [Analyzer] Terminate analysis on OpenMP code instead
of assertion crash (authored by a.sidorin).
Changed prior to commit:
https://reviews.llvm.org/D30565?vs=90441&id=90499#toc
Repository:
rL
a.sidorin added a comment.
Thank you Devin. Should we submit this to 4.0? I guess there are not many users
of both CSA and OpenMP but PR you pointed is already the second report about
this issue I see.
Repository:
rL LLVM
https://reviews.llvm.org/D30565
__
a.sidorin added a comment.
Hello Sean,
It is good to have the ability of recursive lookup. But for me (and I'm sorry),
the code becomes very hard to understand: I need to track if the Decl/DC is in
the source AST or in the AST being imported, if it was imported or if was in
the AST before acro
a.sidorin added a comment.
I have no any objection on this change.
Repository:
rL LLVM
https://reviews.llvm.org/D30798
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
a.sidorin added a comment.
Hello Zoltan,
Thank you for the patch. There is an inline comment.
Comment at: lib/AST/ASTImporter.cpp:5221
IdentifierInfo *ToII = Importer.Import(S->getOutputIdentifier(I));
-if (!ToII)
- return nullptr;
+// ToII is nullptr when no
a.sidorin accepted this revision.
a.sidorin added a comment.
This revision is now accepted and ready to land.
Looks good, thank you!
https://reviews.llvm.org/D30831
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bi
a.sidorin added a comment.
Hello Kareem.
There are some tests in "class-template" dir that may serve as example. You
don't need a warning for a function that was found: you should just return
found declaration. There is already a warning for a declaration that is defined
in multiple TUs. You c
a.sidorin added a comment.
That's excellent. Thank you for this work, Sean!
Comment at: tools/clang-import-test/CMakeLists.txt:19
+ )
\ No newline at end of file
Please add a newline here.
Comment at: tools/clang-import-test/clang-import-te
a.sidorin added a comment.
Thank you Kareem, It looks mostly good, but I'd like to have some functional
tests in ASTMerge for this patch.
Comment at: lib/AST/ASTImporter.cpp:4299
+ if (ImportDeclParts(D, DC, LexicalDC, Name, AlreadyImported, Loc))
+return NULL;
+ assert(
a.sidorin accepted this revision.
a.sidorin added a comment.
This revision is now accepted and ready to land.
LGTM, thanks!
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2577
if (FName == "postEvent" &&
FD->getQualifiedNameAsString() == "QCoreApplication
a.sidorin added inline comments.
Comment at: lib/AST/ASTContext.cpp:1481
+ assert(!FD->hasBody() && "FD has a definition in current translation unit!");
+ if (!FD->getType()->getAs())
+return nullptr; // Cannot even mangle that.
xazax.hun wrote:
> a.sidorin
a.sidorin accepted this revision.
a.sidorin added a comment.
Refactoring changes are always appreciated. I only have a minor naming nit.
Comment at: include/clang/AST/ASTStructuralEquivalence.h:33
+ /// AST contexts for which we are checking structural equivalence.
+ ASTConte
a.sidorin added a comment.
Hi Artem. It is a good point but I think we should have a literal for this
instead.
https://reviews.llvm.org/D32702
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cf
a.sidorin created this revision.
Herald added subscribers: rengolin, aemerson.
Support new AST nodes:
- UnresolvedUsingType
- EmptyDecl
- NamespaceAliasDecl
- UsingDecl
- UsingShadowDecl
- UsingDirectiveDecl
- UnresolvedUsingValueDecl
- UnresolvedUsingTypenameDecl
Refactor error handling in Imp
a.sidorin accepted this revision.
a.sidorin added a comment.
Thank you!
https://reviews.llvm.org/D32702
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
a.sidorin accepted this revision.
a.sidorin added a comment.
This revision is now accepted and ready to land.
Looks good!
Comment at: lib/AST/ASTImporter.cpp:1757
D2->setAnonymousStructOrUnion(true);
+if (PrevDecl) {
+ D2->setPreviousDecl(PrevDecl);
a.sidorin updated this revision to Diff 98441.
a.sidorin added a reviewer: karkhaz.
a.sidorin added a comment.
Address review comments; add the context.
https://reviews.llvm.org/D32751
Files:
lib/AST/ASTImporter.cpp
test/ASTMerge/namespace/Inputs/namespace1.cpp
test/ASTMerge/namespace/Inp
a.sidorin updated this revision to Diff 99668.
a.sidorin added a comment.
Replaced cast<> with cast_or_null<>.
https://reviews.llvm.org/D32751
Files:
lib/AST/ASTImporter.cpp
test/ASTMerge/namespace/Inputs/namespace1.cpp
test/ASTMerge/namespace/Inputs/namespace2.cpp
test/ASTMerge/namespa
a.sidorin updated this revision to Diff 99669.
a.sidorin added a comment.
Removed accidentally duplicated comment.
https://reviews.llvm.org/D32751
Files:
lib/AST/ASTImporter.cpp
test/ASTMerge/namespace/Inputs/namespace1.cpp
test/ASTMerge/namespace/Inputs/namespace2.cpp
test/ASTMerge/nam
a.sidorin added inline comments.
Comment at: lib/AST/ASTImporter.cpp:1311
+ EmptyDecl *ToD = EmptyDecl::Create(Importer.getToContext(), DC, Loc);
+ ToD->setLexicalDeclContext(LexicalDC);
+ LexicalDC->addDeclInternal(ToD);
xazax.hun wrote:
> Don't we need an Im
401 - 465 of 465 matches
Mail list logo
