Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG]: Unsupported compiler overrides are kept when switching compilers, leading to confusing UI issues #5755

Open
DavidSpickett opened this issue Nov 15, 2023 · 2 comments · May be fixed by #6497
Labels

Comments

@DavidSpickett
Copy link
Contributor

Describe the bug

Unsupported overrides are kept when switching compilers which leads to a confusing difference between what the menu shows, the overrides button's active state implies, and what the compiler actually ends up using.

Steps to reproduce

  1. Enable override A for compiler 1
  2. Switch to compiler 2, which does not support override A
  3. The overrides button will be green to show there is an override, but the compiler vetoes it and doesn't pass it to the command line.
  4. If you were to open the overrides menu here, the unsupported override would be deleted.
  5. Switch back to compiler 1
  6. If you did not open the menu while on compiler 2, the override is now back, as compiler 1 supports it. If you did open the menu, it's gone and you would have to put it back.

In the reproducer link, I:

  • Added an "action" override for TableGen.
  • Switched to the gcc compiler for C.
  • You will see that it claims to have an active override for action, but in fact, gcc doesn't support that override.
  • Opening the menu will clear action.

Expected behavior

I have no idea what the expected behaviour here was intended to be, but I can think of 2 models that might work:

  1. Switching to a new compiler removes all unsupported overrides. When you go back to the first compiler you must add them again (draft PR: Remove unsupported overrides when the compiler changes #5747)
  2. Unsupported overrides are kept when switching compilers, simply hidden from the UI, and do not cause the overrides button to be green (draft PR: Put Overrides button in success state only when overrides are actually used #5744)

Option 1 is the simplest to implement, option 2 would mean you can "carry" an option between compilers at least temporarily.

Maybe you compile for target pentiumm with gcc, and switch to MSVC quickly, then go back. It might be nice to have kept the target option so you don't have to add it. But again, I'm not sure what the original intent here is.

The libraries menu is a bit different because it lists only enabled libraries that are compatible with the current compiler. I am not sure if it removes unsupported libraries while doing this though.

Reproduction link

https://godbolt.org/z/aPPeP3bzs

Screenshots

No response

Operating System

No response

Browser version

No response

@OfekShilon
Copy link
Member

@DavidSpickett nice catch. I suspect whenever you say "compiler change" you mean "language change". Is that right?

Anyway my 2c:
Overrides are basically switches and the experience should be consistent with those. Whether intentionally or not, currently when you change a language the original switches are hidden - but when you change back they return. So my vote goes for option 2.

@DavidSpickett
Copy link
Contributor Author

DavidSpickett commented Nov 21, 2023

I like the switches metaphor, that is very concise.

I suspect whenever you say "compiler change" you mean "language change". Is that right?

The effect is more noticeable when switching languages but still happens when switching compilers within the same language. For example GCC for x86 has "target", but clang RISC-V does not. Both within the C language section.

You could argue though that it's less surprising to clear all the overrides on a language change than a compiler change within a language.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants