[flang] Replace ATTRIBUTE_UNUSED with LLVM_ATTRIBUTE_UNUSED.

[ChinouneMehdi]

Summary

ATTRIBUTE_UNUSED wasn't defined with MSVC.

[DavidTruby]

LGTM: We should probably remove this macro and use [[maybe_unused]] directly since it's standard, but I think we can merge this in the mean time to fix MSVC.

[DavidTruby]

@aaron.ballman tried to give a review here but couldn't add himself so he sent this review by email, which I will include here for completeness:

Is there a reason to
not use LLVM_ATTRIBUTE_UNUSED from Compiler.h? (Sorry if this is a
dumb drive-by question.)

In answer to the question (which isn't a dumb one at all!) I would prefer LLVM_ATTRIBUTE_UNUSED to our custom one; however since Flang requires C++17 (unlike most of LLVM) I think we should just use the standard attribute everywhere as it's guaranteed to be available on any C++17 compatible compiler.

[aaron.ballman]

In D78356#1991971, @DavidTruby wrote:

@aaron.ballman tried to give a review here but couldn't add himself so he sent this review by email, which I will include here for completeness:

Thank you for adding this, I think I may be able to comment on the review now (I'll find out for sure when I hit Submit).

Is there a reason to
not use LLVM_ATTRIBUTE_UNUSED from Compiler.h? (Sorry if this is a
dumb drive-by question.)

In answer to the question (which isn't a dumb one at all!) I would prefer LLVM_ATTRIBUTE_UNUSED to our custom one; however since Flang requires C++17 (unlike most of LLVM) I think we should just use the standard attribute everywhere as it's guaranteed to be available on any C++17 compatible compiler.

A compiler can be compatible with C++17 and still ignore this attribute, but may support other spellings of it, which is a (very) minor consideration to keep in mind. More importantly, if the expectation is that the implementation will support the attribute, why is the macro needed at all?

My preference is that we either go with the attribute spelling directly (no macro), or we go with the existing macro in Compilers.h rather than invent a second place for general purpose support macros to live. (If the macro is specific to flang, then I think it makes sense to add to idioms.h, but not for general purpose macros like this.)

flang/include/flang/Common/idioms.h

101–102

Is there a reason to not use LLVM_ATTRIBUTE_UNUSED from Compiler.h? (Sorry if this is a dumb drive-by question.)

[DavidTruby]

Sorry I probably wasn't very clear in what I was trying to say; I completely agree with you! My preference is to go for the attribute directly since it's guaranteed to at least parse in C++17 and I think every compiler we support implements the meaning as well.
If we decide not to do that my second preference is also to go for the existing LLVM_ATTRIBUTE_UNUSED.

However, I'm still in favour of merging this patch, as it is a quick fix for compilers that don't support the attribute we're trying to use at the moment. Moving over to using [[maybe_unused]] directly requires a lot more code modifications (I imagine, I don't actually know how many times ATTRIBUTE_UNUSED is used).

[sscalpone]

LLVM_ATTRIBUTE_UNUSED seems like the right path.

Are there are features of Compiler.h that ought to be used?

[aaron.ballman]

In D78356#1992256, @DavidTruby wrote:

Sorry I probably wasn't very clear in what I was trying to say; I completely agree with you! My preference is to go for the attribute directly since it's guaranteed to at least parse in C++17 and I think every compiler we support implements the meaning as well.
If we decide not to do that my second preference is also to go for the existing LLVM_ATTRIBUTE_UNUSED.

Ah, okay, thank you for clarifying.

However, I'm still in favour of merging this patch, as it is a quick fix for compilers that don't support the attribute we're trying to use at the moment. Moving over to using [[maybe_unused]] directly requires a lot more code modifications (I imagine, I don't actually know how many times ATTRIBUTE_UNUSED is used).

If this is for fixing a broken build then I'm all for landing it in this form right now and handling the churn later.

[ChinouneMehdi]

Please, Someone commit this change.
email: [e-mail]

[sscalpone]

The recommendation is to use LLVM_ATTRIBUTE_UNUSED directly.

[ChinouneMehdi]

Use LLVM_ATTRIBUTE_UNUSED

[aaron.ballman]

LGTM!

[DavidTruby]

LGTM too

[sscalpone]

The original goal was to add [[maybe_unused]]. Thus, this change needs a corresponding update to Compiler.h to add [[maybe_unused]]. I'm ok with you addressing that issue as a separate commit.

[sscalpone]

flang/include/flang/Common/idioms.h

23

Other files have "llvm/" includes above the system includes, e.g. flang/lib/Semantics/mod-file.cpp

[ChinouneMehdi]

Put llvm/include first.

[ChinouneMehdi]

Ping

[DavidTruby]

LGTM

[ChinouneMehdi]

Could someone commit this change, please.

[richard.barton.arm]

Hi @ChinouneMehdi

Looks like this has slipped through the cracks for everyone. I just pushed it now.

I think you should ask Chris for commit access so you don't get forgotten about again :-)

Thanks for the patch!

flang/include/flang/Common/idioms.h

 
 #include <functional>
 #include <list>
 #include <optional>
 #include <string>
 #include <tuple>
 #include <type_traits>
 #include <variant>
+#include "llvm/Support/Compiler.h"
 
 #if __GNUC__ == 7
 // Avoid a deduction bug in GNU 7.x headers by forcing the answer.
 namespace std {
 template <typename A>
 struct is_trivially_copy_constructible<list<A>> : false_type {};
 template <typename A>
 struct is_trivially_copy_constructible<optional<list<A>>> : false_type {};
template <typename A> constexpr bool trait_value() { \
         using U = typename A::T; \
         return U::value; \
       } else { \
         return false; \
       } \
     } \
   } \
   template <typename A> constexpr bool T{class_trait_ns_##T::trait_value<A>()};
 
-#if !defined ATTRIBUTE_UNUSED && (__clang__ || __GNUC__)
-#define ATTRIBUTE_UNUSED __attribute__((unused))
-#endif
-
 // Define enum class NAME with the given enumerators, a static
 // function EnumToString() that maps enumerators to std::string,
 // and a constant NAME_enumSize that captures the number of items
 // in the enum class.
 
 std::string EnumIndexToString(int index, const char *names);
 
 template <typename A> struct ListItemCount {
   constexpr ListItemCount(std::initializer_list<A> list) : value{list.size()} {}
   const std::size_t value;
 };
 
 #define ENUM_CLASS(NAME, ...) \
   enum class NAME { __VA_ARGS__ }; \
-  ATTRIBUTE_UNUSED static constexpr std::size_t NAME##_enumSize{[] { \
+  LLVM_ATTRIBUTE_UNUSED static constexpr std::size_t NAME##_enumSize{[] { \
     enum { __VA_ARGS__ }; \
     return Fortran::common::ListItemCount{__VA_ARGS__}.value; \
   }()}; \
-  ATTRIBUTE_UNUSED static inline std::string EnumToString(NAME e) { \
+  LLVM_ATTRIBUTE_UNUSED static inline std::string EnumToString(NAME e) { \
     return Fortran::common::EnumIndexToString( \
         static_cast<int>(e), #__VA_ARGS__); \
   }
 
 // Check that a pointer is non-null and dereference it
 #define DEREF(p) Fortran::common::Deref(p, __FILE__, __LINE__)
 
 template <typename T> constexpr T &Deref(T *p, const char *file, int line) {