Page MenuHomePhabricator

[Sema] fix false -Wcomma being emitted from void returning functions
ClosedPublic

Authored by inclyc on Aug 15 2022, 7:38 AM.

Diff Detail

Event Timeline

inclyc created this revision.Aug 15 2022, 7:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 15 2022, 7:38 AM
inclyc updated this revision to Diff 452665.Aug 15 2022, 7:44 AM

comments

inclyc published this revision for review.Aug 15 2022, 8:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 15 2022, 8:29 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
inclyc updated this revision to Diff 452691.Aug 15 2022, 8:46 AM

Sync comments of function IgnoreCommaOperand

inclyc retitled this revision from [Sema] fix false -Wcomma being emited from void returning functions to [Sema] fix false -Wcomma being emitted from void returning functions.Aug 15 2022, 8:50 AM

Thank you for working on this fix! I think it's quite close to finished, but it needs some additional test coverage. Also, please add a release note about the fix so users know what's going on.

clang/lib/Sema/SemaExpr.cpp
13978–13980
clang/test/SemaCXX/warn-comma-operator.cpp
151

Can you also add test cases like:

struct S {
  void mem();
};

typedef void Void;
Void typedef_func();

void whatever() {
  S s;
  
  // Member function calls also work as expected.
  s.mem(), int_func();
  // As do lambda calls.
  []() { return; }(), int_func();
  // And we don't get confused about type aliases.
  typedef_func(), int_func();
  // Even function pointers don't confuse us.
  void (*fp)() = void_func;
  fp(), int_func();
}
inclyc updated this revision to Diff 452767.Aug 15 2022, 12:03 PM

Address comments.

Thanks a lot for your suggestion, I noticed that the regression test tested both
C and C++, so I split the test mentioned in the comment into two parts.

inclyc added inline comments.Aug 15 2022, 12:07 PM
clang/docs/ReleaseNotes.rst
74

Do we need an NFC commit to unify the format of issues mentioned here? (Maybe it can be scheduled for a later release). The different ways of citing issues here can be confusing. Issue xxxx # xxx in this context.

aaron.ballman accepted this revision.Aug 16 2022, 5:12 AM
aaron.ballman added a subscriber: tstellar.

LGTM!

clang/docs/ReleaseNotes.rst
74

If we wanted to do something like that, I think we should probably do that as part of the release process as a checklist item (CC @tstellar) but I don't have strong opinions either. We can certainly do it with NFC fixes as folks notice issues if we're fine still having some inconsistency when we release (which I'm fine with, personally).

This revision is now accepted and ready to land.Aug 16 2022, 5:12 AM