Page MenuHomePhabricator

[NFC] Add some assertions to placate my paranoia about sharing a variant bit across FunctionDecl and CXXDeductionGuideDecl - should I do this?
AbandonedPublic

Authored by faisalv on Oct 22 2017, 12:46 PM.

Details

Summary

I'd like to harden my patch here: https://reviews.llvm.org/rL316292 by adding some assertions.

But since the assertions in Decl,.h (FunctionDecl) require knowledge from DeclCXX.h (CXXDeductionGuideDecl),- my question is: In order to keep the member functions inline I factored them out into a separate header file that I included in certain areas.

Is this an acceptable pattern?

Or does anyone have any other preferred engineering suggestions?

Thanks!

Diff Detail

Repository
rL LLVM

Event Timeline

faisalv created this revision.Oct 22 2017, 12:46 PM

Isn't it already an UB if someone set WillHaveBody and but later IsCopyDeductionCandidate being read, vice versa?

Isn't it already an UB if someone set WillHaveBody and but later IsCopyDeductionCandidate being read, vice versa?

Yes that would be UB - but I'm not sure I see how that would happen w the current implementation. Hence I thought I'd add the assertions to be certain.

I'll let Richard comment on whether this pattern is reasonable or not, but I have some very minor nits in the meantime.

include/clang/AST/Decl.h
1683

Since -> Because

(Might as well fix the other one as well while you're at it.)

include/clang/AST/InlineDeclMembers.h
10

This comment seems incorrect.

20

Spurious newline.

Because this functionality will always be for inline Decl members, I think it would make more sense to put the declarations into namespace clang rather than use a qualified name.

34

Another spurious newline.

35

Whitespace is incorrect here.

faisalv updated this revision to Diff 120346.Oct 25 2017, 6:27 PM
faisalv marked 4 inline comments as done.

Incorporated Aaron's feedback (although not sure if I caugh tall the white space issues).

Additionally, I was thinking of reordering the bits - and using UseSEHTry (this bit really makes no sense and even the chance of someone triggering its use erroneously (which can happen with the current bit if someone gives the deduction guide a body, should be zero) as my fusion bit.

I'm also not opposed to just adding a bit to the end of the FunctionDecl bit sequence.

Thanks for taking a look at the patch Aaron!

faisalv abandoned this revision.Nov 11 2017, 10:10 AM

Just added an additional bit-field to FunctionDecl in https://reviews.llvm.org/rL317984

include/clang/AST/InlineDeclMembers.h
35

not sure I follow?