[PATCH] D159256: [NFC][Clang] Remove redundant function definitions

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez added a comment.

Thanks for the review!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159256/new/

https://reviews.llvm.org/D159256

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

[PATCH] D159256: [NFC][Clang] Remove redundant function definitions

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG19550e79b50f: [NFC][Clang] Remove redundant function 
definitions (authored by jmmartinez).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159256/new/

https://reviews.llvm.org/D159256

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGCall.h
  clang/lib/CodeGen/CodeGenModule.h

Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -1259,26 +1259,6 @@
   llvm::AttributeList , unsigned ,
   bool AttrOnCallSite, bool IsThunk);
 
-  /// Adds attributes to F according to our CodeGenOptions and LangOptions, as
-  /// though we had emitted it ourselves.  We remove any attributes on F that
-  /// conflict with the attributes we add here.
-  ///
-  /// This is useful for adding attrs to bitcode modules that you want to link
-  /// with but don't control, such as CUDA's libdevice.  When linking with such
-  /// a bitcode library, you might want to set e.g. its functions'
-  /// "unsafe-fp-math" attribute to match the attr of the functions you're
-  /// codegen'ing.  Otherwise, LLVM will interpret the bitcode module's lack of
-  /// unsafe-fp-math attrs as tantamount to unsafe-fp-math=false, and then LLVM
-  /// will propagate unsafe-fp-math=false up to every transitive caller of a
-  /// function in the bitcode library!
-  ///
-  /// With the exception of fast-math attrs, this will only make the attributes
-  /// on the function more conservative.  But it's unsafe to call this on a
-  /// function which relies on particular fast-math attributes for correctness.
-  /// It's up to you to ensure that this is safe.
-  void mergeDefaultFunctionDefinitionAttributes(llvm::Function ,
-bool WillInternalize);
-
   /// Like the overload taking a `Function &`, but intended specifically
   /// for frontends that want to build on Clang's target-configuration logic.
   void addDefaultFunctionDefinitionAttributes(llvm::AttrBuilder );
Index: clang/lib/CodeGen/CGCall.h
===
--- clang/lib/CodeGen/CGCall.h
+++ clang/lib/CodeGen/CGCall.h
@@ -395,10 +395,25 @@
   bool isExternallyDestructed() const { return IsExternallyDestructed; }
 };
 
-/// Helper to add attributes to \p F according to the CodeGenOptions and
-/// LangOptions without requiring a CodeGenModule to be constructed.
+/// Adds attributes to \p F according to our \p CodeGenOpts and \p LangOpts, as
+/// though we had emitted it ourselves. We remove any attributes on F that
+/// conflict with the attributes we add here.
+///
+/// This is useful for adding attrs to bitcode modules that you want to link
+/// with but don't control, such as CUDA's libdevice.  When linking with such
+/// a bitcode library, you might want to set e.g. its functions'
+/// "unsafe-fp-math" attribute to match the attr of the functions you're
+/// codegen'ing.  Otherwise, LLVM will interpret the bitcode module's lack of
+/// unsafe-fp-math attrs as tantamount to unsafe-fp-math=false, and then LLVM
+/// will propagate unsafe-fp-math=false up to every transitive caller of a
+/// function in the bitcode library!
+///
+/// With the exception of fast-math attrs, this will only make the attributes
+/// on the function more conservative.  But it's unsafe to call this on a
+/// function which relies on particular fast-math attributes for correctness.
+/// It's up to you to ensure that this is safe.
 void mergeDefaultFunctionDefinitionAttributes(llvm::Function ,
-  const CodeGenOptions CodeGenOpts,
+  const CodeGenOptions ,
   const LangOptions ,
   const TargetOptions ,
   bool WillInternalize);
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2002,10 +2002,7 @@
   }
 }
 
-/// Adds attributes to \p F according to our \p CodeGenOpts and \p LangOpts, as
-/// though we had emitted it ourselves. We remove any attributes on F that
-/// conflict with the attributes we add here.
-static void mergeDefaultFunctionDefinitionAttributes(
+void CodeGen::mergeDefaultFunctionDefinitionAttributes(
 llvm::Function , const CodeGenOptions ,
 const LangOptions , const TargetOptions ,
 bool WillInternalize) {
@@ -2065,15 +2062,6 @@
   F.addFnAttrs(FuncAttrs);
 }
 
-void clang::CodeGen::mergeDefaultFunctionDefinitionAttributes(
-llvm::Function , const CodeGenOptions CodeGenOpts,
- 

[PATCH] D159256: [NFC][Clang] Remove redundant function definitions

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 accepted this revision.
jhuber6 added a comment.
This revision is now accepted and ready to land.

In D159256#4630915 , @jmmartinez 
wrote:

> In D159256#4630876 , @jhuber6 wrote:
>
>> In D159256#4630410 , @jmmartinez 
>> wrote:
>>
>>> @jhuber6 I was wondering if there is a reason you kept 3 versions of 
>>> `mergeDefaultFunctionDefinitionAttributes` in 
>>> https://reviews.llvm.org/D152391 ?
>>
>> I believe it's because one was a freestanding function, the other was a 
>> member function, and the last was a common implementation.
>
> Would it be ok if I keep only one? It seems that the member function is not 
> used (I was not sure if there was some external code using it).
>
> If not, I can also keep just 2 versions (the freestanding function and the 
> member function), move the implementation to the freestanding one, and drop 
> the static function since it is redundant.

Yeah I think I noticed that when I was doing the patch but I just left it 
because I figured it would be less disruptive. It should be fine since I'm not 
aware of any other users.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159256/new/

https://reviews.llvm.org/D159256

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

[PATCH] D159256: [NFC][Clang] Remove redundant function definitions

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez added a comment.

In D159256#4630876 , @jhuber6 wrote:

> In D159256#4630410 , @jmmartinez 
> wrote:
>
>> @jhuber6 I was wondering if there is a reason you kept 3 versions of 
>> `mergeDefaultFunctionDefinitionAttributes` in 
>> https://reviews.llvm.org/D152391 ?
>
> I believe it's because one was a freestanding function, the other was a 
> member function, and the last was a common implementation.

Would it be ok if I keep only one? It seems that the member function is not 
used (I was not sure if there was some external code using it).

If not, I can also keep just 2 versions (the freestanding function and the 
member function), move the implementation to the freestanding one, and drop the 
static function since it is redundant.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159256/new/

https://reviews.llvm.org/D159256

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

[PATCH] D159256: [NFC][Clang] Remove redundant function definitions

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added a comment.

In D159256#4630410 , @jmmartinez 
wrote:

> @jhuber6 I was wondering if there is a reason you kept 3 versions of 
> `mergeDefaultFunctionDefinitionAttributes` in 
> https://reviews.llvm.org/D152391 ?

I believe it's because one was a freestanding function, the other was a member 
function, and the last was a common implementation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159256/new/

https://reviews.llvm.org/D159256

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

[PATCH] D159256: [NFC][Clang] Remove redundant function definitions

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez added a comment.

@jhuber6 I was wondering if there is a reason you kept 3 versions of 
`mergeDefaultFunctionDefinitionAttributes` in https://reviews.llvm.org/D152391 ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159256/new/

https://reviews.llvm.org/D159256

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

[PATCH] D159256: [NFC][Clang] Remove redundant function definitions

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez created this revision.
jmmartinez added a reviewer: jhuber6.
Herald added a project: All.
jmmartinez requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

There were 3 definitions of the mergeDefaultFunctionDefinitionAttributes
function: A private implementation, a version exposed in CodeGen, a
version exposed in CodeGenModule.

This patch removes the private and the CodeGenModule versions and keeps
a single definition in CodeGen.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D159256

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGCall.h
  clang/lib/CodeGen/CodeGenModule.h

Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -1259,26 +1259,6 @@
   llvm::AttributeList , unsigned ,
   bool AttrOnCallSite, bool IsThunk);
 
-  /// Adds attributes to F according to our CodeGenOptions and LangOptions, as
-  /// though we had emitted it ourselves.  We remove any attributes on F that
-  /// conflict with the attributes we add here.
-  ///
-  /// This is useful for adding attrs to bitcode modules that you want to link
-  /// with but don't control, such as CUDA's libdevice.  When linking with such
-  /// a bitcode library, you might want to set e.g. its functions'
-  /// "unsafe-fp-math" attribute to match the attr of the functions you're
-  /// codegen'ing.  Otherwise, LLVM will interpret the bitcode module's lack of
-  /// unsafe-fp-math attrs as tantamount to unsafe-fp-math=false, and then LLVM
-  /// will propagate unsafe-fp-math=false up to every transitive caller of a
-  /// function in the bitcode library!
-  ///
-  /// With the exception of fast-math attrs, this will only make the attributes
-  /// on the function more conservative.  But it's unsafe to call this on a
-  /// function which relies on particular fast-math attributes for correctness.
-  /// It's up to you to ensure that this is safe.
-  void mergeDefaultFunctionDefinitionAttributes(llvm::Function ,
-bool WillInternalize);
-
   /// Like the overload taking a `Function &`, but intended specifically
   /// for frontends that want to build on Clang's target-configuration logic.
   void addDefaultFunctionDefinitionAttributes(llvm::AttrBuilder );
Index: clang/lib/CodeGen/CGCall.h
===
--- clang/lib/CodeGen/CGCall.h
+++ clang/lib/CodeGen/CGCall.h
@@ -395,10 +395,25 @@
   bool isExternallyDestructed() const { return IsExternallyDestructed; }
 };
 
-/// Helper to add attributes to \p F according to the CodeGenOptions and
-/// LangOptions without requiring a CodeGenModule to be constructed.
+/// Adds attributes to \p F according to our \p CodeGenOpts and \p LangOpts, as
+/// though we had emitted it ourselves. We remove any attributes on F that
+/// conflict with the attributes we add here.
+///
+/// This is useful for adding attrs to bitcode modules that you want to link
+/// with but don't control, such as CUDA's libdevice.  When linking with such
+/// a bitcode library, you might want to set e.g. its functions'
+/// "unsafe-fp-math" attribute to match the attr of the functions you're
+/// codegen'ing.  Otherwise, LLVM will interpret the bitcode module's lack of
+/// unsafe-fp-math attrs as tantamount to unsafe-fp-math=false, and then LLVM
+/// will propagate unsafe-fp-math=false up to every transitive caller of a
+/// function in the bitcode library!
+///
+/// With the exception of fast-math attrs, this will only make the attributes
+/// on the function more conservative.  But it's unsafe to call this on a
+/// function which relies on particular fast-math attributes for correctness.
+/// It's up to you to ensure that this is safe.
 void mergeDefaultFunctionDefinitionAttributes(llvm::Function ,
-  const CodeGenOptions CodeGenOpts,
+  const CodeGenOptions ,
   const LangOptions ,
   const TargetOptions ,
   bool WillInternalize);
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2002,10 +2002,7 @@
   }
 }
 
-/// Adds attributes to \p F according to our \p CodeGenOpts and \p LangOpts, as
-/// though we had emitted it ourselves. We remove any attributes on F that
-/// conflict with the attributes we add here.
-static void mergeDefaultFunctionDefinitionAttributes(
+void CodeGen::mergeDefaultFunctionDefinitionAttributes(
 llvm::Function , const CodeGenOptions ,
 const LangOptions , const TargetOptions ,
 bool