This is an archive of the discontinued LLVM Phabricator instance.

[ADT] Mark `llvm::array_lengthof` as deprecated
ClosedPublic

Authored by jloser on Sep 8 2022, 8:15 AM.

Details

Summary

As a follow-up of 5e96cea1db0623a833d5376c9ea2ce4528771f97, mark
llvm::array_lengthof as deprecated in favor of using std::size function
directly.

Diff Detail

Event Timeline

jloser created this revision.Sep 8 2022, 8:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 8 2022, 8:15 AM
Herald added a subscriber: StephenFan. · View Herald Transcript
jloser requested review of this revision.Sep 8 2022, 8:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 8 2022, 8:15 AM
kazu accepted this revision.Sep 8 2022, 9:04 AM

As I commented in D133429, please be sure to take care of replacements in internal downstream users before checking this in. LGTM other than that. Thanks!

llvm/include/llvm/ADT/STLArrayExtras.h
31–32

Consider saying something like:

LLVM_DEPRECATED("Use std::size instead.", "std::size")

This way, the compiler warning 'underlines' array_lengthof, which I find very helpful.

This revision is now accepted and ready to land.Sep 8 2022, 9:04 AM
kazu added a comment.Sep 8 2022, 9:08 AM

By the way, I just realized that array_lengthof is the only thing in STLArrayExtras.h. You might want to consider a follow-up patch to stop including the header file (at 7 places or so). Thanks!

jloser updated this revision to Diff 458770.Sep 8 2022, 9:23 AM

Use LLVM_DEPRECATED instead

jloser added a comment.Sep 8 2022, 9:24 AM

By the way, I just realized that array_lengthof is the only thing in STLArrayExtras.h. You might want to consider a follow-up patch to stop including the header file (at 7 places or so). Thanks!

Yep, I plan on doing that and fixing clang and lldb to no longer use this function as well. Once all of the internal callers are using std::size, then I'll land this.

llvm/include/llvm/ADT/STLArrayExtras.h
31–32

Just changed to using LLVM_DEPRECATED — thanks!

MaskRay accepted this revision.Sep 8 2022, 11:57 AM

With my downstream inspection, I think array_lengthof is used much less than Optional::{hasValue,getValue}, etc.
I appreciate that Optional::{hasValue,getValue} had a very long deprecation period so downstream can be more prepared.
array_lengthof removal may not need that long time.

jloser updated this revision to Diff 458843.Sep 8 2022, 1:25 PM

Placement of LLVM_DEPRECATED matters

jloser updated this revision to Diff 459148.Sep 9 2022, 11:42 AM

Include "llvm/Support/Compiler.h" explicitly for using LLVM_DEPRECATED

This revision was automatically updated to reflect the committed changes.

This produces a warning when built with GCC for every single source file that includes the header:

../include/llvm/Support/Compiler.h:145:35: warning: attribute ignored [-Wattributes]  
  145 | #define LLVM_DEPRECATED(MSG, FIX) [[deprecated(MSG)]]
      |                                   ^
../include/llvm/ADT/STLArrayExtras.h:31:18: note: in expansion of macro ‘LLVM_DEPRECATED’        
   31 | constexpr inline LLVM_DEPRECATED("Use std::size instead.", "std::size") size_t
      |                  ^~~~~~~~~~~~~~~
../include/llvm/Support/Compiler.h:145:35: note: an attribute that appertains to a type-specifier is ignored              
  145 | #define LLVM_DEPRECATED(MSG, FIX) [[deprecated(MSG)]]
      |                                   ^
../include/llvm/ADT/STLArrayExtras.h:31:18: note: in expansion of macro ‘LLVM_DEPRECATED’
   31 | constexpr inline LLVM_DEPRECATED("Use std::size instead.", "std::size") size_t
      |                  ^~~~~~~~~~~~~~~
lattner added a subscriber: lattner.Sep 9 2022, 3:41 PM

nice, thanks Joe!

This produces a warning when built with GCC for every single source file that includes the header:

../include/llvm/Support/Compiler.h:145:35: warning: attribute ignored [-Wattributes]  
  145 | #define LLVM_DEPRECATED(MSG, FIX) [[deprecated(MSG)]]
      |                                   ^
../include/llvm/ADT/STLArrayExtras.h:31:18: note: in expansion of macro ‘LLVM_DEPRECATED’        
   31 | constexpr inline LLVM_DEPRECATED("Use std::size instead.", "std::size") size_t
      |                  ^~~~~~~~~~~~~~~
../include/llvm/Support/Compiler.h:145:35: note: an attribute that appertains to a type-specifier is ignored              
  145 | #define LLVM_DEPRECATED(MSG, FIX) [[deprecated(MSG)]]
      |                                   ^
../include/llvm/ADT/STLArrayExtras.h:31:18: note: in expansion of macro ‘LLVM_DEPRECATED’
   31 | constexpr inline LLVM_DEPRECATED("Use std::size instead.", "std::size") size_t
      |                  ^~~~~~~~~~~~~~~

Should be fixed now. The placement before Placement of LLVM_DEPRECATED matters was actually correct for [[deprecated(...)]] :)

jloser added a comment.Sep 9 2022, 4:01 PM

This produces a warning when built with GCC for every single source file that includes the header:

../include/llvm/Support/Compiler.h:145:35: warning: attribute ignored [-Wattributes]  
  145 | #define LLVM_DEPRECATED(MSG, FIX) [[deprecated(MSG)]]
      |                                   ^
../include/llvm/ADT/STLArrayExtras.h:31:18: note: in expansion of macro ‘LLVM_DEPRECATED’        
   31 | constexpr inline LLVM_DEPRECATED("Use std::size instead.", "std::size") size_t
      |                  ^~~~~~~~~~~~~~~
../include/llvm/Support/Compiler.h:145:35: note: an attribute that appertains to a type-specifier is ignored              
  145 | #define LLVM_DEPRECATED(MSG, FIX) [[deprecated(MSG)]]
      |                                   ^
../include/llvm/ADT/STLArrayExtras.h:31:18: note: in expansion of macro ‘LLVM_DEPRECATED’
   31 | constexpr inline LLVM_DEPRECATED("Use std::size instead.", "std::size") size_t
      |                  ^~~~~~~~~~~~~~~

Should be fixed now. The placement before Placement of LLVM_DEPRECATED matters was actually correct for [[deprecated(...)]] :)

Thanks for the quick fix, @MaskRay — I appreciate it. By the way, the extra includes for STLArrayExtras.h in the LLVM tree is addressed with https://reviews.llvm.org/D133600