meson icon indicating copy to clipboard operation
meson copied to clipboard

compilers: Use a dataclass instead of tuple for cached results

Open dcbaker opened this issue 1 year ago • 3 comments

There's been a number of bugs where the code does something like:

def foo(...) -> T.Tuple[bool, bool]: ...

if foo():
   ...

Which will always resolve to True since a non-empty Tuple is truthy. To avoid that I've moved to a simple struct that implements a bool dunder, so that doing if result: does the expected thing.

@eli-schwartz, here is one option, as we discussed. I don't think it makes sense to wrap this in another level of abstraction just to drop the the cached argument.

dcbaker avatar Apr 17 '24 20:04 dcbaker

simple struct that implements a bool dunder, so that doing if result: does the expected thing.

TBH I've always found overridden __bool__() feels counterintuitive and don't like relying on it. I was expecting to always use the .result member...

eli-schwartz avatar Apr 17 '24 21:04 eli-schwartz

Also it's somewhat difficult to eyeball the changes due to the large number of docstrings added, which really should go in a separate commit.

eli-schwartz avatar Apr 17 '24 21:04 eli-schwartz

I've split the docstring and comment changes into a separate patch. I'm concerned about the __bool__ thing, since the real value here is that the class implements an explicit bool dunder instead of being truthy, which would mean that there is no benefit to using the struct and we should just leave it, since if CompileCheckResult(...) will always resolve to True, no matter what value is used.

dcbaker avatar Apr 18 '24 17:04 dcbaker