Conversation
|
FWIW, I like this! |
| :param enable: If true then enable the warning, otherwise suppress it. | ||
| """ | ||
|
|
||
| target: str |
There was a problem hiding this comment.
I think an enable member went missing here.
There was a problem hiding this comment.
Oops, I the enable docstring is a copy-paste error. There is not -Wno-error=... in any compiler AFAIK, so no need to represent it.
There was a problem hiding this comment.
s/any/every/ ?
So I guess yeah, if it can't be universally represented it might as well be opaque whenever it appears.
There was a problem hiding this comment.
Originally I had Warning() with both enable and error fields, but realized that you could end up with the invalid state of error disabled. I guess we could leave it and have some kind of validation pass later, but I prefer to not be able to model invalid state if possible.
There was a problem hiding this comment.
-Wno-error is valid at least on gcc and clang. Given f.c:
int main()
{
puts("aa");
}you have:
$ gcc f.c -Wno-error=implicit-function-declaration -std=c99
f.c: In function ‘main’:
f.c:3:9: warning: implicit declaration of function ‘puts’ [-Wimplicit-function-declaration]
$ clang f.c -Wno-error=implicit-function-declaration -std=c99
f.c:3:2: warning: call to undeclared function 'puts'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
Without the flag you get an error with both compilers.
There was a problem hiding this comment.
Interesting, GCC does not document that on their warnings page, just -W, -Wnoand-Werror=`
Since it does exist I'll fold it.
There was a problem hiding this comment.
?
-Werror=
Make the specified warning into an error. The specifier for a warning is appended; for
example -Werror=switch turns the warnings controlled by -Wswitch into errors. This switch
takes a negative form, to be used to negate -Werror for specific warnings; for example
-Wno-error=switch makes -Wswitch warnings not be errors, even when -Werror is in effect.
There was a problem hiding this comment.
Maybe you could have an enum DISABLED, DEFAULT_LEVEL, WARNING, ERROR corresponding to -Wno-, -W, -Wno-error=, -Werror=.
There was a problem hiding this comment.
Ah, I was looking at the htmlized version of the info page, It's probably there but not readily obvious
Which I've somehow failed to add all of these years.
The goal is to be able to convert arguments from raw arguments that are passed in via the `c_args` and friends methods, as well as from options and the environment, into an abstract IR. This IR is then passed around internally, and the backend is responsible for lowering it into the format that it wants. I'm hoping that this will simplify some amount of passing arguments between languages (like C arguments being passed to non-c compilers, and linker arguments between languages) as well as simplify the non-ninja backends which generally use a different mechanism than raw compiler arguments.
…arguments These will be used to convert the arguments in many cases.
This is only for dynamic linkers.
bdccf01 to
fd3bc13
Compare
|
@dnicolodi thanks for the reviews! I've fixed the things you pointed out. Also in the latest update, linker arguments are now handled, along with a this the check for manually specified -rpath arguments has gotten better, as it no longer needs to check for strings, but can instead just see if there are any instances of There's still a lot of plumbing work to do here, apart from implementing all of this for various compilers. |
| elif arg.startswith('-U'): | ||
| ret.append(arguments.Undefine(arg.removeprefix('-U'))) | ||
| elif arg.startswith('-l'): | ||
| ret.append(arguments.LinkerSearch(arg.removeprefix('-l'))) |
|
|
||
|
|
||
| @dataclasses.dataclass | ||
| class Undefine: |
There was a problem hiding this comment.
For warnings we have enable=True, why not the same for Define?
There was a problem hiding this comment.
In fact since (EDIT: fixed) -Dfoo and -Dfoo=1 are the same, could Undefine('foo') be replaced by Define('foo', value=None)?
There was a problem hiding this comment.
They are NOT the same, one sets it to implicitly 1 and the other, explicitly the null string.
For both, ifdef says "yes it is defined" and redefining may result in compiler warnings.
There was a problem hiding this comment.
Yeah, I really want to make it impossible to represent invalid state. So we basically have three distinct cases:
set define to value
set define to nothing
remove define
So... would representing it as:
class Define:
name: str
value: str | boolwork correctly?
You'd have value = False mean -Ufoo value = True mean -Dfoo and value = mean -Dfoo=<str>?
There was a problem hiding this comment.
You're right, sorry. I meant that -Dfoo and -Dfoo=1 could be canonicalized to a single form (Define('foo', '1') in the abstract form, whereas the concrete form could be either) and value=None could be used for undefining.
There was a problem hiding this comment.
Maybe, but would that mean the user sets -Dfoo=1 and we translate to -Dfoo (or vice versa)?
There was a problem hiding this comment.
Yes... OTOH you do want to collapse -Dfoo -Dfoo=1 to just one of them. Setting value='1' would also make __eq__ easier to implement.
Maybe some kind of have_equals: bool could be added to make round-trip conversion possible, and it would be ignored by __eq__.
There was a problem hiding this comment.
I don't know that we have to guarantee we pass exactly the same arguments as long as we pass equivalent arguments. Especially since we already attempt to do some level of de-duplication. I'm perfectly fine with -Dfoo= becoming -Dfoo=1.
| k, v = arg.split('=') | ||
| else: | ||
| k, v = arg, None | ||
| ret.append(arguments.Define(k, v)) |
There was a problem hiding this comment.
Defines can be spelled on the command line as -D KEY=value (with the space after the -D switch) meaning that they are seen as two arguments, not as one. Thus, I think that arguments parsing needs to be more sophisticated. Maybe using something like argparse may make the code simpler.
There was a problem hiding this comment.
It's possible we will have to do something more sophisticated before this can land, right now my biggest concern is getting to the point that i can prove that the IR can do everything we currently do with the POSIX style string parsing, and solve some of the difficult problems we can't solve easily. So, for the moment I'd rather not get hung up here, but I will leave these opened to keep them in mind.
There was a problem hiding this comment.
This is also an issue we'll need to resolve for Rust because --cfgs must be passed with a space in them.
| k, v = arg, None | ||
| ret.append(arguments.Define(k, v)) | ||
| elif arg.startswith('-U'): | ||
| ret.append(arguments.Undefine(arg.removeprefix('-U'))) |
|
#15577 should be revisited once this work is complete. |
This is set to draft because it's very much a Work in Progress.
The way we handle compiler arguments in Meson is to manipulate raw strings. This works fine when the majority of your compilers are the same, or at least attempt to be compatible. Unfortunately, Meson has to deal with many different toolchains, which often have incompatible argument syntax.
This manifests in a number of different ways:
-Wl,-rlinkor-rlinkinside ofbuild.py, but that could miss-rlinkfor many compilers.ifblocks converting string command line arguments to XML fieldsIn order to address this I propose moving to an abstract IR for these arguments. This abstract IR would simplify de-duplicating and cleaning arguments, as well as converting their forms. Each Compiler provides a mechanism to convert arguments from their format into the abstract IR, and a method to convert the abstract IR back into concrete arguments in their expected format. We then manipulate the abstract IR more easily. Consider something like this:
In this format it's pretty easy to look and see that we have two copies of
-DXand two copies of-Wfooalong with-Wno-foo, cancelling both of those out.Below is a very trivial implementation of this idea, with just enough implemented for GCC or Clang to compile the
1 trivialtest (run directly). This also is making use of python features we currently cant rely on, I'll rewrite those later.Known TODO items:
-Dand-Uwith GCC/Clang)--cfgwith Rust)