Page MenuHomePhabricator

[llvm] Use std::size instead of llvm::array_lengthof
ClosedPublic

Authored by jloser on Sep 7 2022, 8:54 AM.

Details

Summary

LLVM contains a helpful function for getting the size of a C-style
array: llvm::array_lengthof. This is useful prior to C++17, but not as
helpful for C++17 or later: std::size already has support for C-style
arrays.

Change call sites to use std::size instead.

Diff Detail

Event Timeline

jloser created this revision.Sep 7 2022, 8:54 AM
Herald added a reviewer: ributzka. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
jloser requested review of this revision.Sep 7 2022, 8:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 7 2022, 8:54 AM
MaskRay accepted this revision.Sep 7 2022, 6:23 PM
This revision is now accepted and ready to land.Sep 7 2022, 6:23 PM
dblaikie added a subscriber: kazu.Sep 7 2022, 11:38 PM

As a follow-up, probably good to deprecate the API with the necessary recommended alternative for at least a little while, to client code migrations easier.

@kazu - this might be something you'd be interested in helping with, or pointing @jloser to the right tools/directions.

This revision was landed with ongoing or failed builds.Sep 8 2022, 8:02 AM
This revision was automatically updated to reflect the committed changes.
jloser added a comment.Sep 8 2022, 8:17 AM

As a follow-up, probably good to deprecate the API with the necessary recommended alternative for at least a little while, to client code migrations easier.

@kazu - this might be something you'd be interested in helping with, or pointing @jloser to the right tools/directions.

Sounds good to me. I just opened https://reviews.llvm.org/D133502 to mark the function deprecated, but let me know if you or @kazu had something else in mind. I've fixed downstream callers in MLIR and LLDB (https://reviews.llvm.org/D133501) with Clang being on my list to do as well. This only accounts for the call sites in LLVM codebase though rather than external downstream. Is there a process (e.g. wait two weeks after deprecating before removal) or something of the like?

kazu added a comment.Sep 8 2022, 8:57 AM

As a follow-up, probably good to deprecate the API with the necessary recommended alternative for at least a little while, to client code migrations easier.

@kazu - this might be something you'd be interested in helping with, or pointing @jloser to the right tools/directions.

Sounds good to me. I just opened https://reviews.llvm.org/D133502 to mark the function deprecated, but let me know if you or @kazu had something else in mind. I've fixed downstream callers in MLIR and LLDB (https://reviews.llvm.org/D133501) with Clang being on my list to do as well. This only accounts for the call sites in LLVM codebase though rather than external downstream. Is there a process (e.g. wait two weeks after deprecating before removal) or something of the like?

This is a great cleanup. Thank you!

I am not aware of any documented process. For my recent deprecation of Optional::hasValue, I am waiting for 3 months or so, but some call it too long. So, wait as long as reasonable in your terms.

In any event, I think it's a good idea to clean up internal downstream users first before deprecating array_lengthof. The llvm codebase and internal downstream build cleanly even with -Werror (with clang being the host compiler), so we don't want to introduce a lot of deprecation warnings all of a sudden.

(yep, @kazu's covered everything I can think of here - I'll leave it to you two to work it through)