This is an archive of the discontinued LLVM Phabricator instance.

[CMake][runtimes] Skip the generic target variable if named matches
AbandonedPublic

Authored by phosek on Dec 19 2019, 5:19 PM.

Details

Summary

The existing logic relies on ordering, so if you use the following:

RUNTIMES_target+multi_VARIABLE_NAME
RUNTIMES_target_VARIABLE_NAME

The generic target variable would override the target+multi one. To
avoid that issue, we skip matching the generic target if the named
matches.

Diff Detail

Event Timeline

phosek created this revision.Dec 19 2019, 5:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 19 2019, 5:19 PM
JamesNagurne added a comment.EditedDec 20 2019, 1:07 PM

This foreach loop goes over each variable, yes?
If so, then I'm not sure this change does what is required.

If the list variableNames is:

RUNTIMES_target+multi_XYZ
RUNTIMES_target_XYZ

This loop will:

  1. variableName = RUNTIMES_target+multi_XYZ
  2. Check if {name} matches, it does
  3. Add -DXYZ=value to the list
  4. variableName = RUNTIMES_target_XYZ
  5. Check if {name} matches, it doesn't
  6. Else, check if {target} matches, it does
  7. Add -DXYZ=value to the list, overriding the expected value in 3 above

Edit: That is to say, only one path will ever be executed per iteration of this loop, so the addition of else() doesn't change the behavior.

phosek abandoned this revision.Dec 20 2019, 5:05 PM

This foreach loop goes over each variable, yes?
If so, then I'm not sure this change does what is required.

If the list variableNames is:

RUNTIMES_target+multi_XYZ
RUNTIMES_target_XYZ

This loop will:

  1. variableName = RUNTIMES_target+multi_XYZ
  2. Check if {name} matches, it does
  3. Add -DXYZ=value to the list
  4. variableName = RUNTIMES_target_XYZ
  5. Check if {name} matches, it doesn't
  6. Else, check if {target} matches, it does
  7. Add -DXYZ=value to the list, overriding the expected value in 3 above

Edit: That is to say, only one path will ever be executed per iteration of this loop, so the addition of else() doesn't change the behavior.

You're right, this doesn't address the issue. I was hoping to come up with a way to short circuit the iteration if a more specific variable was already added, but it's difficult to track that. Given that, your solution is probably the easiest one so let's go with that.