Page MenuHomePhabricator

[flang] Replace ATTRIBUTE_UNUSED with LLVM_ATTRIBUTE_UNUSED.
AcceptedPublic

Authored by ChinouneMehdi on Apr 17 2020, 3:44 AM.

Details

Reviewers
jdoerfert
DavidTruby
sscalpone
aaron.ballman
Group Reviewers
Restricted Project
Summary

ATTRIBUTE_UNUSED wasn't defined with MSVC.

Diff Detail

Event Timeline

ChinouneMehdi created this revision.Apr 17 2020, 3:44 AM
ChinouneMehdi created this object with edit policy "Restricted Project (Project)".
Herald added a project: Restricted Project. · View Herald Transcript
sscalpone accepted this revision.Apr 17 2020, 7:17 AM
This revision is now accepted and ready to land.Apr 17 2020, 7:17 AM
DavidTruby accepted this revision.Apr 17 2020, 8:17 AM

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 changed the edit policy from "Restricted Project (Project)" to "All Users".

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

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

LLVM_ATTRIBUTE_UNUSED seems like the right path.

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

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.

Please, Someone commit this change.
email: chinoune.mehdi@hotmail.com

The recommendation is to use LLVM_ATTRIBUTE_UNUSED directly.

ChinouneMehdi retitled this revision from [flang] Use a better definition for ATTRIBUTE_UNUSED to [flang] Replace ATTRIBUTE_UNUSED with LLVM_ATTRIBUTE_UNUSED..

Use LLVM_ATTRIBUTE_UNUSED

DavidTruby accepted this revision.Apr 30 2020, 6:30 AM

LGTM too

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 added inline comments.Apr 30 2020, 8:34 AM
flang/include/flang/Common/idioms.h
24

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

Put llvm/include first.

ChinouneMehdi marked an inline comment as done.Apr 30 2020, 10:07 AM
ChinouneMehdi edited subscribers, added: flang-commits; removed: llvm-commits.May 1 2020, 12:38 PM
sscalpone accepted this revision.Mon, May 4, 11:24 PM