This is an archive of the discontinued LLVM Phabricator instance.

[llvm-config] Canonicalize CMake booleans to 0/1
ClosedPublic

Authored by mgorny on Jan 5 2017, 10:30 AM.

Details

Summary

Following the similar change to lit configuration, ensure that all CMake
booleans are canonicalized to 0/1 when being passed to llvm-config. This
fixes the incorrect interpretation of values when user passes another
value than the ON/OFF, and simplifies the code by removing unnecessary
string matching.

Furthermore, the code for --has-rtti and --has-global-isel has been
modified to print consistent values indepdently of the boolean used by
passed by the user to CMake. Sadly, the code already implicitly used
different values for the two (YES/NO for --has-rtti, ON/OFF for
--has-global-isel).

Diff Detail

Repository
rL LLVM

Event Timeline

mgorny updated this revision to Diff 83269.Jan 5 2017, 10:30 AM
mgorny retitled this revision from to [llvm-config] Canonicalize CMake booleans to 0/1.
mgorny updated this object.
mgorny added a subscriber: llvm-commits.
beanz accepted this revision.Jan 9 2017, 3:23 PM
beanz edited edge metadata.

Can you please add a test case? Otherwise LGTM.

This revision is now accepted and ready to land.Jan 9 2017, 3:23 PM
mgorny added a comment.Jan 9 2017, 4:01 PM

What kind of a test case? Like checking if those boolean options return correct values?

beanz added a comment.Jan 9 2017, 5:52 PM

If I understand correctly, the patch changed the behavior of the --has-rtti and --has-global-isel options to standardize them. It would be nice to have that in a test case.

mgorny updated this revision to Diff 83811.Jan 10 2017, 8:05 AM
mgorny edited edge metadata.

@beanz, is that fine or did I go too far testing all multi-value options? ;-)

Also, is there a better way for case-insensitive matching with FileCheck?

Tests look good.

Sadly I don't think we have a way to expose case insensitivity in the expression, and we don't surface insensitivity through FileCheck.

I don't think it would be hard to add a CHECK-INSENSITIVE option if you thought it would be useful, but I wouldn't require you to do it just for this patch.

LGTM! Thanks!
-Chris

This revision was automatically updated to reflect the committed changes.