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

Refactoring and enhancements to RGBASM warnings #1526

Merged
merged 2 commits into from
Oct 4, 2024

Conversation

Rangi42
Copy link
Contributor

@Rangi42 Rangi42 commented Sep 30, 2024

Fixes #1527

  • Allow a no- prefix to negate "meta" warnings (-Wno-all, -Wno-extra, -Wno-everything)
  • Allow -Wno-error to prevent -Werror from making specific individual or meta warnings into errors

Needs more test cases, but ready to review as-is. (test/asm/warn-numeric-string.asm covers a lot already.)

(And yay, -55 LoC!)

@Rangi42 Rangi42 added bug Unexpected behavior / crashes; to be fixed ASAP! enhancement Typically new features; lesser priority than bugs rgbasm This affects RGBASM labels Sep 30, 2024
@Rangi42 Rangi42 added this to the v0.9.0 milestone Sep 30, 2024
@Rangi42 Rangi42 requested a review from ISSOtm September 30, 2024 05:53
@ISSOtm
Copy link
Member

ISSOtm commented Sep 30, 2024

I'm not sure that I like OPT w. In what situations would someone want to disable all warnings, programmatically? (I'm already not super fond of the option in the first place.)

@aaaaaa123456789
Copy link
Member

I'm not sure that I like OPT w. In what situations would someone want to disable all warnings, programmatically? (I'm already not super fond of the option in the first place.)

Consistency. The mental model for OPT is significantly simpler when you don't have to guess at which options it supports.
I can definitely see it used for shorter code when you want to temporarily disable a few warnings.

@ISSOtm
Copy link
Member

ISSOtm commented Sep 30, 2024

But several options are not supported already: -E, for example.

I don't think that OPT should support all CLI options, either, because some of them don't make sense beyond the initial CLI invocation (why use OPT DDUCK=🦆 when def DUCK equs "🦆" is right there?); this would mean that there will always be some inconsistency, and that thus the line should be drawn somewhere.

@aaaaaa123456789
Copy link
Member

Sure, "this option makes no sense" is a reasonable argument. "This option is perfectly functional and meaningful, but I don't like it" is a very dubious one.

@ISSOtm
Copy link
Member

ISSOtm commented Sep 30, 2024

OPT DDUCK makes sense (“define a string symbol called DUCK and whose contents are 1), and is perfectly functional since EQUS is. It is purely a “I don't like that it would create two string definition syntaxes”.

The rationale for OPT is not “allow changing all/some CLI flags at runtime”, but “allow persisting some changes to the input's processing“.

OPT b is “well, I like my %0010_1010s, but I'd like to use something more visually clear for this one-off 1bpp graphic”. OPT g is the same.

OPT p made sense before we supported 2+-arg ds, and has been kept only for compat since, despite seeing near-zero use (AFAIA, anyway).

OPT W is “this specific line of code triggers a false positive that I'd like to keep enabled for the rest of the program, and I'd like not to get a warning printed on every single build”.

I could go on, but to me, every single one of the currently-supported OPT flags has a reason to be toggled at runtime, beyond “there's an equivalent CLI switch”. And I don't see OPT w passing that test, hence why I am feeling iffy about its addition.

@aaaaaa123456789
Copy link
Member

I have absolutely no issues with OPT DDUCK=1, for the record.

@Rangi42
Copy link
Contributor Author

Rangi42 commented Sep 30, 2024

My two reasons for including OPT w support were consistency and to allow disabling all warnings programmatically. You're right that we're not totally consistent about "all CLI options are OPT options" (and don't need to be), and OPT Wno-everything will allow disabling everything anyway, so I'm okay with removing OPT w if you still prefer that.

@aaaaaa123456789
Copy link
Member

I'd say there's a clear line that you can have: things that can be pushed and popped. PUSHO/POPO are critical to the safe usage of OPT, so you probably don't want to add any flags that wouldn't be easy to push and pop. Things like -E fall in this bin.
On the other hand, w is perfectly pushable and poppable.

@Rangi42 Rangi42 marked this pull request as draft September 30, 2024 15:15
@Rangi42
Copy link
Contributor Author

Rangi42 commented Sep 30, 2024

This is the pseudocode logic we should follow, after #1527 is also implemented:

if -Wno-foobar {
	return DISABLED;
} else if -Werror=foobar {
	return ERROR;
} else if -Wfoobar {
	if -Werror and not -Wno-error=foobar and not covered by -Wno-error=meta {
		return ERROR;
	} else {
		return ENABLED;
	}
} else if covered by -Wno-meta {
	return DISABLED;
} else if covered by -Werror=meta {
	return ERROR;
} else if covered by -Wmeta {
	if -Werror and not -Wno-error=foobar and not covered by -Wno-error=meta {
		return ERROR;
	} else {
		return ENABLED;
	}
} else if disabled by default {
	return DISABLED;
} else if errored by default { // never actually happens but technically possible
	return ERROR;
} else { // enabled by default
	if -Werror and not -Wno-error=foobar and not covered by -Wno-error=meta {
		return ERROR;
	} else {
		return ENABLED;
	}
}

@Rangi42 Rangi42 force-pushed the warnings branch 2 times, most recently from 1e2b856 to de746c6 Compare September 30, 2024 19:53
@Rangi42 Rangi42 marked this pull request as ready for review September 30, 2024 19:54
@Rangi42 Rangi42 force-pushed the warnings branch 3 times, most recently from 4b5cc8b to b44c1fb Compare September 30, 2024 20:02
@Rangi42 Rangi42 changed the title Fix and extend RGBASM warnings: Refactoring and enhancements to RGBASM warnings Sep 30, 2024
@Rangi42 Rangi42 force-pushed the warnings branch 3 times, most recently from 8dc0bb4 to a6afe01 Compare September 30, 2024 20:24
@Rangi42 Rangi42 removed the bug Unexpected behavior / crashes; to be fixed ASAP! label Sep 30, 2024
@Rangi42
Copy link
Contributor Author

Rangi42 commented Sep 30, 2024

So, -w is not meant as an alias for -Wno-everything; it's meant to simply disable all warnings, regardless of -W... or current OPT W... state. I think that's fine as just a CLI flag.

@Rangi42 Rangi42 force-pushed the warnings branch 2 times, most recently from 0916f3f to f9ed4a3 Compare September 30, 2024 22:13
@Rangi42 Rangi42 force-pushed the warnings branch 4 times, most recently from 1cce123 to e003803 Compare October 2, 2024 03:58
* Allow a `no-` prefix to negate "meta" warnings
  (`-Wno-all`, `-Wno-extra`, `-Wno-everything`)
* Allow `-Wno-error=...` to override `-Werror`
  (including for "meta" warnings)
Copy link
Member

@ISSOtm ISSOtm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't fully grok the implementation, but Sylvie walked me through the logic, and it seems sound. That's enough for me, so, merging.

@ISSOtm ISSOtm merged commit cf85146 into gbdev:master Oct 4, 2024
22 checks passed
@Rangi42 Rangi42 deleted the warnings branch October 4, 2024 19:55
@Rangi42 Rangi42 restored the warnings branch October 4, 2024 19:55
@Rangi42 Rangi42 deleted the warnings branch October 4, 2024 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Typically new features; lesser priority than bugs rgbasm This affects RGBASM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhancements to warning diagnostics
3 participants