This is an archive of the discontinued LLVM Phabricator instance.

Fix false positive unsequenced access and modification warning in array subscript expression.
ClosedPublic

Authored by stryku on Aug 15 2018, 4:59 AM.

Diff Detail

Repository
rC Clang

Event Timeline

stryku created this revision.Aug 15 2018, 4:59 AM
hiraditya added inline comments.
lib/Sema/SemaChecking.cpp
11678

Is this comment still relevant?

stryku updated this revision to Diff 160885.Aug 15 2018, 12:18 PM

Thanks for pointing that out. You're probably right, these two calls are self-explanatory.

Rakete1111 requested changes to this revision.Aug 16 2018, 8:15 PM
Rakete1111 added a subscriber: Rakete1111.

Your patch breaks a lot of stuff in the test suite. For example:

void f() {
  int A = 0;
  (A++, A) = 1; // warning from this patch, but this is perfectly valid since forever.
}

Also, you don't take into account the fact that the rule you mention was added in C++17. In versions prior to that (and C!), the warning is not a false positive.

This revision now requires changes to proceed.Aug 16 2018, 8:15 PM
stryku updated this revision to Diff 161943.Aug 22 2018, 6:47 AM

@Rakete1111 Thank you for the comments. You're perfectly right, there was an issue in my code. I've fixed it and also E1 and E2 will be sequenced only in C++ >= 17.

Test coverage?
Also, please always upload all patches with full context (-U99999)

stryku added a comment.EditedAug 22 2018, 7:27 AM

Thanks for the advice. Should I update a new diff with -U99999?

About tests. I've probably missed something but I looked through the project and didn't find any tests for unsequenced operations. If you know a place where such tests are placed, please point it and I'll add the one for this code.
If there are no tests for unsequenced operations, should I introduce them?

EDIT
Sorry, I've found the tests. I'll cover my implementation there.

stryku updated this revision to Diff 162585.Aug 26 2018, 11:14 AM

Added test cases for the code.

Here I think I need your assistance. AFAIK in C++ 17 a couple more sequence related rules appeared. If we would like to cover them in current warn-unsequenced.cpp file, I think we would end up with a little mess with ifdefs for C++ < 17 and C++ >= 17. That's why I have introduced a new file warn-unsequenced-cxx17.cpp which covers my code and more tests will be added to it while their implementation as well. What do you think?

What do you think?

Good idea!

test/SemaCXX/warn-unsequenced-cxx17.cpp
8

Oh no, you forgot a semicolon there! :)

stryku updated this revision to Diff 162630.Aug 27 2018, 1:34 AM

Added missing semicolon.

stryku marked 2 inline comments as done.Aug 31 2018, 1:53 AM

@Rakete1111 or anyone else, any more comments? (:

test/SemaCXX/warn-unsequenced-cxx17.cpp
8

Sorry. I remember that I had this problem and I've resolved it. Apparently I've sent wrong patch file.

stryku marked an inline comment as done.Sep 11 2018, 11:57 AM

Not sure if you guys are still reviewing this. Do you have any more thoughts? If no, what should I do to move further with the patch?

Friendly ping (:

stryku added a comment.Oct 4 2018, 2:57 AM

@Rakete1111 Any more comments?

Sorry, for some reason I didn't see your updates.

Can you add a test for C++17? Then your patch is good to go! :)

Rakete1111 accepted this revision.Oct 10 2018, 3:30 PM

Nevermind my last comment, I was tired. LGTM

This revision is now accepted and ready to land.Oct 10 2018, 3:30 PM
rsmith accepted this revision.Oct 10 2018, 3:41 PM

Thanks, LGTM!

Are you interested in fixing the other cases for which p0145 tightened evaluation order (., ->, .*, ->*, <<, >>, callee in a function call, assignment and compound assignment) too? =)

lebedev.ri added inline comments.Oct 10 2018, 11:03 PM
test/SemaCXX/warn-unsequenced-cxx17.cpp
2

One last-minute thought: this is only a positive test.
You don't test what happens before C++17.

Rakete1111 added inline comments.Oct 11 2018, 2:50 AM
test/SemaCXX/warn-unsequenced-cxx17.cpp
2

It is tested. Look at the diff for test/SemaCXX/warn-unsequenced.cpp :)
Or are you suggesting to merge the two files?

lebedev.ri added inline comments.Oct 11 2018, 2:56 AM
test/SemaCXX/warn-unsequenced-cxx17.cpp
2

I see that the negative test is in warn-unsequenced.cpp, but the positive test is in warn-unsequenced-cxx17.cpp.
This split is the reason of my remark.
I'm not sure if this is an issue, or if merging them is the solution.

stryku marked an inline comment as done.Oct 11 2018, 12:37 PM
stryku added inline comments.
test/SemaCXX/warn-unsequenced-cxx17.cpp
2

Like I said in some of the previous comments, merging these files, AFAIU, would introduce a little ifdefing. With other sequence-related topics from p0145 there would be a lot more ifdefing introduced, and IMHO the file would be a lot less readable. That's why I've introduced this file for C++17+.

stryku added a comment.EditedOct 11 2018, 12:43 PM

@rsmith Of course. Not sure what's better: to submit one patch per case or submit all of them in one patch (didn't deal much with them, not sure how big will the change be)?

@rsmith Of course. Not sure what's better: to submit one patch per case or submit all of them in one patch (didn't deal much with them, not sure how big will the change be)?

It's really up to you. I don't think a patch that does them all would be too big to review, and doing N small reviews isn't really much more work than one larger review, so please go with whichever approach best suits your workflow.

Hi again, since I don't have commit rights, could you please merge this patch into the project sources? Thanks in advance.

This revision was automatically updated to reflect the committed changes.