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

[Faisal Vali via Phabricator via cfe-commits]

faisalv abandoned this revision.
faisalv added a comment.

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




Comment at: include/clang/AST/InlineDeclMembers.h:35
+
+#endif  //LLVM_CLANG_AST_INLINEDECLMEMBERS_H
+

aaron.ballman wrote:
> Whitespace is incorrect here.
not sure I follow?



Repository:
  rL LLVM

https://reviews.llvm.org/D39166



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Faisal Vali via Phabricator via cfe-commits]

faisalv updated this revision to Diff 120346.
faisalv marked 4 inline comments as done.
faisalv added a comment.

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!


Repository:
  rL LLVM

https://reviews.llvm.org/D39166

Files:
  include/clang/AST/Decl.h
  include/clang/AST/InlineDeclMembers.h
  include/clang/Sema/Sema.h
  lib/AST/Decl.cpp
  lib/AST/DeclCXX.cpp
  lib/Parse/ParseCXXInlineMethods.cpp
  lib/Sema/SemaDecl.cpp

Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -12175,7 +12175,8 @@
 
   // See if this is a redefinition. If 'will have body' is already set, then
   // these checks were already performed when it was set.
-  if (!FD->willHaveBody() && !FD->isLateTemplateParsed()) {
+  if (!isa(FD) && !FD->willHaveBody() &&
+  !FD->isLateTemplateParsed()) {
 CheckForFunctionRedefinition(FD, nullptr, SkipBody);
 
 // If we're skipping the body, we're done. Don't enter the scope.
@@ -12186,7 +12187,8 @@
   // Mark this function as "will have a body eventually".  This lets users to
   // call e.g. isInlineDefinitionExternallyVisible while we're still parsing
   // this function.
-  FD->setWillHaveBody();
+  if (!isa(FD))
+FD->setWillHaveBody();
 
   // If we are instantiating a generic lambda call operator, push
   // a LambdaScopeInfo onto the function stack.  But use the information
@@ -12373,7 +12375,8 @@
 
   if (FD) {
 FD->setBody(Body);
-FD->setWillHaveBody(false);
+if (!isa(FD))
+  FD->setWillHaveBody(false);
 
 if (getLangOpts().CPlusPlus14) {
   if (!FD->isInvalidDecl() && Body && !FD->isDependentContext() &&
Index: lib/Parse/ParseCXXInlineMethods.cpp
===
--- lib/Parse/ParseCXXInlineMethods.cpp
+++ lib/Parse/ParseCXXInlineMethods.cpp
@@ -13,6 +13,7 @@
 
 #include "clang/Parse/Parser.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/AST/InlineDeclMembers.h"
 #include "clang/Parse/ParseDiagnostic.h"
 #include "clang/Parse/RAIIObjectsForParser.h"
 #include "clang/Sema/DeclSpec.h"
Index: lib/AST/DeclCXX.cpp
===
--- lib/AST/DeclCXX.cpp
+++ lib/AST/DeclCXX.cpp
@@ -18,6 +18,7 @@
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
+#include "clang/AST/InlineDeclMembers.h"
 #include "clang/AST/ODRHash.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/Basic/IdentifierTable.h"
Index: lib/AST/Decl.cpp
===
--- lib/AST/Decl.cpp
+++ lib/AST/Decl.cpp
@@ -23,6 +23,7 @@
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
+#include "clang/AST/InlineDeclMembers.h"
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/AST/Stmt.h"
 #include "clang/AST/TypeLoc.h"
Index: include/clang/Sema/Sema.h
===
--- include/clang/Sema/Sema.h
+++ include/clang/Sema/Sema.h
@@ -22,6 +22,7 @@
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprObjC.h"
 #include "clang/AST/ExternalASTSource.h"
+#include "clang/AST/InlineDeclMembers.h"
 #include "clang/AST/LocInfoType.h"
 #include "clang/AST/MangleNumberingContext.h"
 #include "clang/AST/NSAPI.h"
Index: include/clang/AST/InlineDeclMembers.h
===
--- include/clang/AST/InlineDeclMembers.h
+++ include/clang/AST/InlineDeclMembers.h
@@ -0,0 +1,39 @@
+//===- InlineDeclMembers.h - Decl.h Members that must be inlined -*- C++ -*-==//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+//  This file defines inline members of classes defined in Decl.h that have
+//  (circular) dependencies on other files and should only be included
+//  in .cpp files (as opposed to .h).
+//
+//===--===//
+
+#ifndef LLVM_CLANG_AST_INLINEDECLMEMBERS_H
+#define LLVM_CLANG_AST_INLINEDECLMEMBERS_H
+
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclCXX.h"
+namespace clang {
+
+inline bool FunctionDecl::willHaveBody() const {
+  assert(!isa(this) &&
+ "must not be called on a deduction guide since we share this 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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




Comment at: include/clang/AST/Decl.h:1683
   // Since a Deduction Guide [C++17] will never have a body, we can share the
-  // storage, and use a different name.
+  // storage, and use a different name.  Since WillHaveBody is not serialized 
we
+  // don't need to worry about collisions there.

Since -> Because

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



Comment at: include/clang/AST/InlineDeclMembers.h:10
+//
+//  This file defines the Decl subclasses.
+//

This comment seems incorrect.



Comment at: include/clang/AST/InlineDeclMembers.h:20
+
+
+inline bool clang::FunctionDecl::willHaveBody() const {

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.



Comment at: include/clang/AST/InlineDeclMembers.h:34
+
+
+#endif  //LLVM_CLANG_AST_INLINEDECLMEMBERS_H

Another spurious newline.



Comment at: include/clang/AST/InlineDeclMembers.h:35
+
+#endif  //LLVM_CLANG_AST_INLINEDECLMEMBERS_H
+

Whitespace is incorrect here.


Repository:
  rL LLVM

https://reviews.llvm.org/D39166



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Faisal Vali via Phabricator via cfe-commits]

faisalv added a comment.

In https://reviews.llvm.org/D39166#903131, @lichray wrote:

> 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.


Repository:
  rL LLVM

https://reviews.llvm.org/D39166



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Zhihao Yuan via Phabricator via cfe-commits]

lichray added a comment.

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


Repository:
  rL LLVM

https://reviews.llvm.org/D39166



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Faisal Vali via Phabricator via cfe-commits]

faisalv created this revision.
faisalv added a project: clang.
Herald added a subscriber: eraman.

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!


Repository:
  rL LLVM

https://reviews.llvm.org/D39166

Files:
  include/clang/AST/Decl.h
  include/clang/AST/InlineDeclMembers.h
  include/clang/Sema/Sema.h
  lib/AST/Decl.cpp
  lib/AST/DeclCXX.cpp
  lib/Parse/ParseCXXInlineMethods.cpp

Index: lib/Parse/ParseCXXInlineMethods.cpp
===
--- lib/Parse/ParseCXXInlineMethods.cpp
+++ lib/Parse/ParseCXXInlineMethods.cpp
@@ -13,6 +13,7 @@
 
 #include "clang/Parse/Parser.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/AST/InlineDeclMembers.h"
 #include "clang/Parse/ParseDiagnostic.h"
 #include "clang/Parse/RAIIObjectsForParser.h"
 #include "clang/Sema/DeclSpec.h"
Index: lib/AST/DeclCXX.cpp
===
--- lib/AST/DeclCXX.cpp
+++ lib/AST/DeclCXX.cpp
@@ -18,6 +18,7 @@
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
+#include "clang/AST/InlineDeclMembers.h"
 #include "clang/AST/ODRHash.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/Basic/IdentifierTable.h"
Index: lib/AST/Decl.cpp
===
--- lib/AST/Decl.cpp
+++ lib/AST/Decl.cpp
@@ -23,6 +23,7 @@
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
+#include "clang/AST/InlineDeclMembers.h"
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/AST/Stmt.h"
 #include "clang/AST/TypeLoc.h"
Index: include/clang/Sema/Sema.h
===
--- include/clang/Sema/Sema.h
+++ include/clang/Sema/Sema.h
@@ -22,6 +22,7 @@
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprObjC.h"
 #include "clang/AST/ExternalASTSource.h"
+#include "clang/AST/InlineDeclMembers.h"
 #include "clang/AST/LocInfoType.h"
 #include "clang/AST/MangleNumberingContext.h"
 #include "clang/AST/NSAPI.h"
Index: include/clang/AST/InlineDeclMembers.h
===
--- include/clang/AST/InlineDeclMembers.h
+++ include/clang/AST/InlineDeclMembers.h
@@ -0,0 +1,37 @@
+//===- InlineDeclMembers.h - Decl.h Members that must be inlined -*- C++ -*-==//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+//  This file defines the Decl subclasses.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_AST_INLINEDECLMEMBERS_H
+#define LLVM_CLANG_AST_INLINEDECLMEMBERS_H
+
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclCXX.h"
+
+
+inline bool clang::FunctionDecl::willHaveBody() const {
+  assert(!isa(this) &&
+"must not be called on a deduction guide since we share this data "
+"member while giving it different semantics");
+  return WillHaveBody;
+}
+inline void clang::FunctionDecl::setWillHaveBody(bool V) {
+  assert(!isa(this) &&
+"must not be called on a deduction guide since we share this data "
+"member while giving it different semantics");
+  WillHaveBody = V;
+}
+
+
+#endif  //LLVM_CLANG_AST_INLINEDECLMEMBERS_H
+
+
Index: include/clang/AST/Decl.h
===
--- include/clang/AST/Decl.h
+++ include/clang/AST/Decl.h
@@ -1680,7 +1680,9 @@
 
 protected:
   // Since a Deduction Guide [C++17] will never have a body, we can share the
-  // storage, and use a different name.
+  // storage, and use a different name.  Since WillHaveBody is not serialized we
+  // don't need to worry about collisions there.
+
   union {
 /// Indicates if the function declaration will have a body, once we're done
 /// parsing it.
@@ -2074,8 +2076,8 @@
   void setHasSkippedBody(bool Skipped = true) { HasSkippedBody = Skipped; }
 
   /// True if this function will eventually have a body, once it's fully parsed.
-  bool willHaveBody() const { return WillHaveBody; }
-  void setWillHaveBody(bool V = true) { WillHaveBody = V; }
+  bool willHaveBody() const;
+  void setWillHaveBody(bool V = true);
 
   void setPreviousDeclaration(FunctionDecl * PrevDecl);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org