Comment by philsnow
3 years ago
AST diffs instead of textual diffs might have helped here (to spot the `.` making the code un-compilable).
Edit: oof, though the stray character in question is inside a perfectly legitimate C string, so to catch this, any such diffs would need to Matroyshka down and that seems unsolvable / intractable.
> Edit: oof, though the stray character in question is inside a perfectly legitimate C string, so to catch this, any such diffs would need to Matroyshka down and that seems unsolvable / intractable.
Not sure, you could also just forbid code that's too complex to analyse without going down a rabbit hole. Instead of trying to go down the rabbit hole.
In general, it's hard to analyse arbitrary code. But we don't have to allow arbitrarily complex code when doing code review.
I think main issue was that it was embedded in the file itself like that. Would have preferred to have it in a separate valid C file with syntax highlighting etc and being parsed from that file.
Perhaps, but given how most build systems work, that would complicate things in other ways (since build systems often try to compile all .c files).
That's really not how most build systems work.
I am a CMake novice. Is that true for CMake in this example?
1 reply →
Put it in a special folder that is ignored from the build.
Ah yes but thanks to C being cursed due to includes and macros this is harder to do
Huh, the code with a dot is not legal C. It is CMake issue that the test breaks here.
That’s what makes this so clever: these systems were born in the era where you couldn’t trust anything - compilers sometimes emitted buggy code, operating systems would claim to be Unix but had weird inconsistencies on everything from system calls to command line tool arguments, etc. - so they just try to compile a test script and if it fails assume that the feature isn’t supported. This is extremely easy to miss since the tool is working as it’s designed and since it’s running tons of stuff there’s a ton of noise to sort through to realize that something which failed was supposed to have worked.
3 replies →
That's kind of the point. It's feature detection code. If the code cleanly compiles, the feature is assumed supported, otherwise, it's assumed not present/functional. This pretty common with autotools. The gotcha here is this innocuous period is not supposed to be syntactically valid. It's meant to be subtle and always disable the feature.
2 replies →