This is an archive of the discontinued LLVM Phabricator instance.

Move from llvm::makeArrayRef to ArrayRef deduction guides - part 1
ClosedPublic

Authored by serge-sans-paille on Jan 3 2023, 7:17 AM.

Details

Summary

Since we're now requiring C++17, Let's get rid of makeXXX functions like makeArrayRef, and use deduction guides instead.

Apart from codebase modernization, there isn't much benefit from that
move, but I can still mention that it would slightly (probably
negligibly) decrease the number of symbols / debug info, as deduction
guides don't generate new code.

The only non-automatic changes have been:

  1. Write the deduction guides
  2. ArrayRef(some_uint8_pointer, 0) needs to be changed into ArrayRef(some_uint8_pointer, (size_t)0) to avoid an ambiguous call with ArrayRef((uint8_t*), (uint8_t*))
  3. CVSymbol sym(makeArrayRef(symStorage)); needed to be rewritten as CVSymbol sym{ArrayRef(symStorage)}; otherwise the compiler is confused and thinks we have a (bad) function prototype. There was a few similar situation across the codebase.
  4. ADL doesn't seem to work the same for deductio-guides and functions, so at some point the llvm namespace must be explicitly stated.
  5. The "reference mode" of makeArrayRef(ArrayRef<T> &) that acts as no-op is not supported (a constructor cannot achieve that).

Diff Detail

Event Timeline

Herald added a reviewer: shafik. · View Herald Transcript
Herald added a reviewer: rriddle. · View Herald Transcript
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a reviewer: MaskRay. · View Herald Transcript
Herald added a reviewer: jpienaar. · View Herald Transcript
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added a reviewer: rafauler. · View Herald Transcript
Herald added a reviewer: Amir. · View Herald Transcript
Herald added a reviewer: maksfb. · View Herald Transcript
Herald added a reviewer: NoQ. · View Herald Transcript
Herald added a reviewer: njames93. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
serge-sans-paille requested review of this revision.Jan 3 2023, 7:17 AM
Herald added a reviewer: herhut. · View Herald Transcript
Herald added a reviewer: dcaballe. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
mehdi_amini added inline comments.Jan 3 2023, 7:52 AM
llvm/include/llvm/ADT/ArrayRef.h
502

Can we keep the makeArrayRef functions for now and mark them deprecated?

jpienaar added inline comments.Jan 3 2023, 11:15 AM
llvm/include/llvm/ADT/ArrayRef.h
502

+1 that would also allow for this change to broken up so that the deduction guides land first and then the updates (potentially even 3 hops, 1) add guides, 2) update, 3) mark deprecated - 1 & 2 could be combined but it'll greatly simplify review/enable sharding it)

+ keep deprecated version, flagged appropriately

MaskRay accepted this revision.Jan 3 2023, 3:55 PM

LGTM if makeArrayRef is kept in this patch. Do we foresee compiler bugs (for supported compilers) in this area? If there may be a risk, consider migrate a part of the code base first.

Split the original patch in pieces: first just introduce the deduction guide.

This revision was not accepted when it landed; it landed in state Needs Review.Jan 3 2023, 11:20 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
serge-sans-paille retitled this revision from [WIP] Move from llvm::makeArrayRef to ArrayRef deduction guides to Move from llvm::makeArrayRef to ArrayRef deduction guides - part 1.Jan 3 2023, 11:32 PM