Page MenuHomePhabricator

Avoid redundant inline with LLVM_ATTRIBUTE_ALWAYS_INLINE
ClosedPublic

Authored by jpark37 on Aug 4 2020, 6:06 PM.

Details

Summary

Fix MSVC warning when __forceinline is paired with inline.

Diff Detail

Event Timeline

jpark37 created this revision.Aug 4 2020, 6:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2020, 6:06 PM
jpark37 requested review of this revision.Aug 4 2020, 6:06 PM
jpark37 updated this revision to Diff 283095.Aug 4 2020, 6:32 PM

Trying to fix author

jpark37 updated this revision to Diff 283114.Aug 4 2020, 8:16 PM

Running clang-format

jpark37 updated this revision to Diff 283115.Aug 4 2020, 8:20 PM

Trying to clang-format again

The clang-tidy warnings seem out of scope.

This looks like maybe it's going in the opposite direction from 2e4ca848f45f08fec084e886280a2e45e48b6e1b - might be interesting to understand why the issue that patch was addressing aren't applicable anymore. (& why this change is now needed when the code's been this way for years?)

jpark37 updated this revision to Diff 284552.Aug 10 2020, 7:11 PM

Add inline to always_inline

jpark37 updated this revision to Diff 284563.Aug 10 2020, 7:26 PM

Add inline to always_inline attribute.

jpark37 updated this revision to Diff 284564.Aug 10 2020, 7:28 PM

Restore entire diff. I think this should be correct finally.

Verified attribute((always_inline)) should be paired with inline. I think this should work.

To answer the question, it's just a warning, so my guess is that people chose to ignore it. Or maybe it only used to trigger for inline + inline, and not __forceinline + inline.

It's specifically C4141: https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-1-c4141

jpark37 updated this revision to Diff 284770.Aug 11 2020, 9:07 AM

Changed "attribute((always_inline)) inline" to "inline attribute((always_inline))" to match Mesa: https://gitlab.freedesktop.org/mesa/mesa/-/blob/35938c15e22e3021f7693425f0d2134845c81f6b/src/util/macros.h#L144

dblaikie accepted this revision.Aug 11 2020, 11:24 AM

Fair enough - thanks!

This revision is now accepted and ready to land.Aug 11 2020, 11:24 AM

Is there something I need to do to land this? I'm new to LLVM.

This revision was automatically updated to reflect the committed changes.

Is there something I need to do to land this? I'm new to LLVM.

Once a patch is approved, helps to ask to have it committed - otherwise folks tend to assume you have commit access and will commit it yourself. (I've gone ahead and committed this - it should automatically update the phab review and link the commit)

(details here: https://llvm.org/docs/Contributing.html#how-to-submit-a-patch "If you do not have commit access, please let people know during the review and someone should commit it on your behalf.")

Also, if you could upload future patches with full context that's handy (arc will do this for you, but if you are going to submit reviews via the web interface, this discusses how to include more context)