This is an archive of the discontinued LLVM Phabricator instance.

[OpaquePtr][Clang] Add CLANG_ENABLE_OPAQUE_POINTERS cmake option
ClosedPublic

Authored by nikic on Apr 5 2022, 5:50 AM.

Details

Summary

This option controls whether -opaque-pointers or -no-opaque-pointers is the default. Once opaque pointers are enabled by default, this will provide a simple way to temporarily opt-out of the change.

Diff Detail

Event Timeline

nikic created this revision.Apr 5 2022, 5:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2022, 5:50 AM
Herald added a subscriber: mgorny. · View Herald Transcript
nikic requested review of this revision.Apr 5 2022, 5:50 AM
nikic added a comment.Apr 5 2022, 6:13 AM

We might want to only actually land this one when the default gets switched -- I believe that cmake has this confusing behavior where changing an option default doesn't actually have an effect if the option has already been cached. IIRC when we switched to the NewPM this left a lot of people still using the LegacyPM by accident.

We might want to only actually land this one when the default gets switched -- I believe that cmake has this confusing behavior where changing an option default doesn't actually have an effect if the option has already been cached. IIRC when we switched to the NewPM this left a lot of people still using the LegacyPM by accident.

Yeah, bit unfortunate - I wonder if there's a way to add an option with 3 states, "default" (which we could change the semantics of), ON and OFF (so someone can explicitly opt in before the switch, and can explicitly opt out before the switch so they don't have to bundle a switch at the same time as the change, etc)?

nikic added a comment.Apr 6 2022, 1:32 AM

We might want to only actually land this one when the default gets switched -- I believe that cmake has this confusing behavior where changing an option default doesn't actually have an effect if the option has already been cached. IIRC when we switched to the NewPM this left a lot of people still using the LegacyPM by accident.

Yeah, bit unfortunate - I wonder if there's a way to add an option with 3 states, "default" (which we could change the semantics of), ON and OFF (so someone can explicitly opt in before the switch, and can explicitly opt out before the switch so they don't have to bundle a switch at the same time as the change, etc)?

I think something like this would work:

# Manually handle default so we can change the meaning of a cached default. 
set(CLANG_ENABLE_OPAQUE_POINTERS "DEFAULT" CACHE STRING
    "Enable opaque pointers by default")
if(CLANG_ENABLE_OPAQUE_POINTERS STREQUAL "DEFAULT")
  set(CLANG_ENABLE_OPAQUE_POINTERS_INTERNAL OFF)
elseif(CLANG_ENABLE_OPAQUE_POINTERS)
  set(CLANG_ENABLE_OPAQUE_POINTERS_INTERNAL ON)
else()
  set(CLANG_ENABLE_OPAQUE_POINTERS_INTERNAL OFF)
endif()

This still has the annoying behavior that if you once specified -DCLANG_ENABLE_OPAQUE_POINTERS=false and then later drop it, you'll still keep implicitly using the old behavior, until you remove the build directory entirely. But I don't think one can do anything about that part.

nikic updated this revision to Diff 420738.Apr 6 2022, 1:38 AM

Add intermediate option to make sure we can change a cached default value lateron.

I checked that modifying the default and running ninja does have an effect, unlike in the previous version.

aeubanks accepted this revision.Apr 6 2022, 8:56 AM
This revision is now accepted and ready to land.Apr 6 2022, 8:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 7 2022, 1:15 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript