This is an archive of the discontinued LLVM Phabricator instance.

Keep the IfStmt node even if the condition is invalid
AbandonedPublic

Authored by ogoffart on Oct 1 2015, 6:41 AM.

Details

Summary

This is quite useful for IDE's or other tools which would like to recover as
much as possible even if there are a few errors in the code, and discarding
the full 'if' bodies is unfortunate.

Diff Detail

Event Timeline

ogoffart updated this revision to Diff 36235.Oct 1 2015, 6:41 AM
ogoffart retitled this revision from to Keep the IfStmt node even if the condition is invalid .
ogoffart updated this object.
ogoffart added a reviewer: cfe-commits.
milianw edited edge metadata.Oct 1 2015, 7:27 AM

Is there still an error reported for the invalid condition?

Anyhow, from my POV this is excellent and should fix a bunch of issues I've seen when using clang-c in KDevelop. I never got around to investigating it, but it always was related to conditionals. I'm pretty sure this patch fixes it then.

@ogoffart: For Manuel to see the difference, could you share the before/after screenshots of your code browser? That makes the impact of this patch pretty clear.

+Richard, whom we need to validate AST changes to make sure they're benign.

rsmith added inline comments.Oct 7 2015, 3:23 PM
lib/Sema/SemaStmt.cpp
508–509

This will create an IfStmt with no ConditionExpr. That is not a valid AST construct, and it's not reasonable to expect all the downstream consumers of the AST to be able to cope with it. It's not hard to find parts of the codebase that will crash when presented with this:

lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:1133
lib/Analysis/Consumed.cpp:1265
lib/Sema/AnalysisBasedWarnings.cpp:740

... and so on.

As the comment above suggests, you could create some sort of placeholder expression node here instead (either use something like an OpaqueValueExpr, or add a new Expr subclass to represent an erroneous expression -- the latter would probably be useful in many cases where we hit parse errors but can still produce some useful information to tools).

Are the analysis run if there is an error? I think the consumer should expect null condition anyway.

But i'll try to add an ErrorExpr anyway.

ogoffart updated this revision to Diff 37035.Oct 10 2015, 2:55 PM
ogoffart edited edge metadata.

The updated patch uses OpaqueValueExpr

rsmith added inline comments.Oct 10 2015, 3:53 PM
lib/Sema/SemaStmt.cpp
505–506

Please add a comment here saying that we're creating this node for error recovery. Also, the type of the expression should be BoolTy, not VoidTy. Other than that, this looks fine to me.

ogoffart abandoned this revision.Oct 11 2015, 10:36 AM
ogoffart marked an inline comment as done.

commited as r249982. (I forgot to add the link to reviews.llvm.org in the commit message. I'll do it next time)