This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Remove prefer-dynamic-value test override
ClosedPublic

Authored by kastiglione on Aug 22 2022, 8:55 AM.

Details

Summary

Remove the test override of target.prefer-dynamic-value.

Previously, the lldb default was no-dynamic-values. In rG9aa7e8e9ffbe (in
2015), the default was changed to no-run-target, but at that time the tests
were changed to be run with no-dynamic-value. I don't the reasons for not
changing the tests, perhaps to avoid determining which tests to change and how.

Because no-run-target is the lldb default, I think it makes sense to make it
the test default too. It puts the test config closer to what's used in
practice.

This change removes the target.prefer-dynamic-value override, and for those
tests that failed, they have been updated to explicitly use
no-dynamic-values. Future changes could update these tests to use dynamic
values too, or they can be left as is to exercise non-dynamic typing.

Diff Detail

Event Timeline

kastiglione created this revision.Aug 22 2022, 8:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 22 2022, 8:55 AM
kastiglione requested review of this revision.Aug 22 2022, 8:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 22 2022, 8:55 AM
kastiglione added inline comments.Aug 22 2022, 9:28 AM
lldb/test/API/sanity/TestSettingSkipping.py
15–32

these tests had nothing to do with prefer-dynamic-value, they picked something to decorate on. I picked something else to decorate on. I couldn't pick a setting now that configuration.settings is empty.

mib added a comment.Aug 22 2022, 1:34 PM

were changed to be run with no-dynamic-value. I don't the reasons for not
changing the tests, perhaps to avoid determining which tests to change and how.

I don't know the reasons ... ?

mib accepted this revision.Aug 22 2022, 1:36 PM

I've had this issue in the past in a test where the variable value would not match what I'd get on an interactive debug session, until I added that flag.

I think this makes sense to have consistency between the interactive session default and the test suite. LGTM!

This revision is now accepted and ready to land.Aug 22 2022, 1:36 PM
augusto2112 accepted this revision.Aug 22 2022, 1:40 PM

I think this is a good change. One related thing that we could look into fixing is that currently both run-target and no-run-target do the exact same thing. Not sure if we could remove one of them since this could break user's settings, but we could at least document it.

Fixed description

This revision was landed with ongoing or failed builds.Aug 22 2022, 3:46 PM
This revision was automatically updated to reflect the committed changes.