Multi-decl overhaul (including inline struct fixes) #657
Conversation
mattmccutchen-cci
force-pushed
the
inline-struct-fixes
branch
from
b183451 to
0035579
Compare
Now that we automatically name them: - When -alltypes is on, we no longer need to warn that the multi-decl may be rewritten incorrectly. - When -alltypes is off, we no longer generate constraints to try to avoid the need to rewrite the multi-decl at all. InlineStructDetector did one thing unrelated to its name: it constrained all union fields wild. Move that logic to ProgramInfo::addVariable, make the constraint reason message more specific, and update root_cause.c to expect the new message.
The problem already affected several regression tests for more complex cases (which expected the extra space), and we didn't seem to notice or care. But now it is affecting every unchanged variable of that form in a multi-decl that gets rewritten, and this broke several other regression tests.
multi-decl rewriting splits them rather than losing them. Fixes #531 except for a "declaration does not declare anything" compiler warning that will be addressed by de-nesting the inline structs.
A fixup to "Get the correct form...".
declaration as if it were a definition. Fixes #644.
The existing regression tests pass, but more testing is needed.
More are still needed.
Delete the test of "Unable to rewrite a typedef with multiple names" from root_cause.c because that case no longer causes wildness. (I think it makes sense to just delete the test.)
Now that this case works, add a few tests of it.
In the process, fix a bug that I found in the lookup of the type of an unchanged typedef.
|
I'm still getting lots of errors from libtiff because a typedef struct name doesn't exist. They all relate to |
TODO: Add regression tests.
I added a fix to this PR. Thanks again for the report! |
|
Ping me when you've merged this branch with the |
This is much nicer than temporarily mutating the VarDecl to remove the initializer.
…nto inline-struct-fixes
|
Benchmark run from action branch That branch is under review but should mean that all regression tests pass and all benchmark failures are the result of bounds problems or have GitHub issues filed for them already. |
…nto inline-struct-fixes
|
Last call for review of this PR. I think the only notable changes I made since John's approval are:
The regression tests passed on my machine, and the benchmark tests are running now. If the benchmark tests pass, I'm ready to merge this PR. |
Results look good to me. I'm happy to see this merged. |
With this change, all callers of emitSupplementaryDeclarations want it to add a newline at the beginning and not the end, so we can remove the extra newline there. - Change getNextComma back to getNextCommaOrSemicolon as it was before PR #657. - Some other comment fixes and minor refactorings. Notably: in emitSupplementaryDeclarations, change the InsertTextAfter call to the equivalent InsertText to try to avoid confusion with InsertTextAfterToken.
As of 2021-07-29, this change still has a ways to go before it's ready for review, but I thought I was overdue to file a draft PR to make it easier to find, now that I'm pretty confident that the
inline-struct-fixesbranch is the one I'll be submitting. I'd welcome early feedback, though be aware that I'm already planning to redo some parts of the change and I'll likely rewrite the commit history, which would make previously added line-level review comments harder to access.Completed and planned work for this PR:
DeclStmts. (John and I agreed that we can base multi-decl detection on identical startSourceLocations in all cases, rather than keeping a separate code path forDeclStmts in the hope that it would be more reliable because it is more explicit.)staticstorage class from a struct definition split from a multi-decl to avoid a compiler warning (Splitting ofstaticmulti-decl containing inline struct leaves the structstatic, generating compiler warning #647).3c -alltypesdeletes bodies of unnamed struct types #542, Multi-decls containing unnamed inline structs may be orphaned even without-alltypes#646).mkStringinstead ofDecl::printfor unchanged multi-decl names only if they need to use a new name for an unnamed inline struct (to minimize our exposure tomkStringbugs: see Various bugs inmkString#703). This no longer fixes issue 652. (Ironically, if I write Multi-decl rewriting loses macro in base type of variable whose type did not change #652, GitHub treats it as a closing keyword: it doesn't understand English-language negation.)Fix aRemoved from this PR since it is no longer important since we don't usemkStringformatting bug that was exposed by the previous item.mkStringfor unchanged members without an unnamed inline struct. Update 2021-09-28: Now submitted in mkString fixes (mostly moved from the multi-decl overhaul) #714.TagDecls: struct, union, and enum. (The non-struct cases may be uncommon, but I think they're worth trying to address while I'm here.)Change multi-decl rewriting to preserve the entire original text of unmodified members, if feasible (a further improvement in the spirit of Multi-decl rewriting loses macro in base type of variable whose type did not change #652).Update 2021-09-23: I'm dropping this because it looks like it will probably add significant complexity for little benefit. We can always do it later if we care enough.mkStringForDeclWithUnchangedTypeand-itypes-for-extern.