================
Comment at: lib/AST/Decl.cpp:2569-2570
@@ -2568,1 +2568,4 @@
 
+// \brief The combination of the extern and inline keywords under MSVC forces
+// the function to be required.
+bool FunctionDecl::isMSExternInline() const {
----------------
`///`, not `//`.

================
Comment at: lib/AST/Decl.cpp:2571
@@ +2570,3 @@
+// the function to be required.
+bool FunctionDecl::isMSExternInline() const {
+  const ASTContext &Context = getASTContext();
----------------
It's a little odd for this to check for `extern` but not `inlined`. Maybe at 
least assert that the function `isInlined` and mention that in the comment?

================
Comment at: lib/AST/Decl.cpp:2588
@@ +2587,3 @@
+      return false;
+  return Redecl->getStorageClass() == SC_Extern;
+}
----------------
Maybe move this check to the start of the function to avoid walking the redecls 
in the common case that the function isn't `extern`?

================
Comment at: lib/AST/Decl.cpp:2610-2612
@@ -2587,5 +2609,5 @@
 ///
 /// Specifically, this determines if adding the current declaration to the set
 /// of redeclarations of the given functions causes
 /// isInlineDefinitionExternallyVisible to change from false to true.
 bool FunctionDecl::doesDeclarationForceExternallyVisibleDefinition() const {
----------------
The corresponding update to `isInlineDefinitionExternallyVisible` seems to be 
missing.

================
Comment at: lib/CodeGen/CodeGenModule.cpp:561-562
@@ +560,4 @@
+    // replaced, the function definition cannot be discarded.
+    if (D->getMostRecentDecl()->isMSExternInline())
+      return llvm::Function::WeakODRLinkage;
+
----------------
Is there any risk that we'll emit IR for the function definition before we see 
the 'extern' declaration? (Will we ever need to patch up the linkage on an 
existing IR function?)


http://llvm-reviews.chandlerc.com/D3207

BRANCH
  master

ARCANIST PROJECT
  clang
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Reply via email to