Page MenuHomePhabricator

[FuncSpec][NFC] Refactor internal structures.

Authored by labrinea on Mar 1 2022, 10:30 AM.



Factored out part of as requested on the review.

Diff Detail

Event Timeline

labrinea created this revision.Mar 1 2022, 10:30 AM
labrinea requested review of this revision.Mar 1 2022, 10:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2022, 10:30 AM

Looks like a good refactoring, but needs a clang-format, and have added one question inline.


Was wondering if we could change SpecializationList to:


so that we move and transfer ownership instead of copying things here.

Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 1:44 AM
fhahn added a subscriber: fhahn.Mar 2 2022, 2:00 AM
fhahn added inline comments.

Does the function modify Arg? If not, please either pass by value or as const reference.

labrinea added inline comments.Mar 2 2022, 5:43 AM

Will do, thanks.


Good remark, but will there really be a copy? I thought that the Return Value Optimization (copy elision) prevents that. Alternatively I could make the function void and pass the SpecializationList by reference on the argument list.

SjoerdMeijer added inline comments.Mar 2 2022, 7:26 AM

My understanding is that this is an optimisation that may or may not be implemented. Looks like to be specified for c++17 though, but since we are using C++14 being explicit might be better.

labrinea updated this revision to Diff 412426.Mar 2 2022, 8:25 AM

Changes from last revision:

  • marked ArgInfo as const reference when passed to markArgInFuncSpecialization() and rewriteCallSites()
  • passed SpecializationList by reference to calculateGains() instead of returning by value
  • removed the IsPartial flag from isArgumentInteresting() and getPossibleConstants() as it's no longer used anywhere in the code
  • clang-formatted the code
SjoerdMeijer accepted this revision.Mar 3 2022, 1:51 AM

Thanks, looks like a good change to me.

This revision is now accepted and ready to land.Mar 3 2022, 1:51 AM
This revision was landed with ongoing or failed builds.Mar 3 2022, 5:09 AM
This revision was automatically updated to reflect the committed changes.