[PATCH] D144273: [clang][ASTImporter] Add VaList declaration to lookup table.

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

@balazske Thanks for clarifying the side effects of the current solution and I support creating that side-effect-free getter function -- it sounds to be a good solution for this problem. One minor doubt: I can theoretically imagine the case when the TU object doesn't contain the VaList declaration yet when the constructor of ASTImporterLookupTable is called (so the side-effect-free call does nothing), but then it somehow creates duplicated decls by importing VaList twice (e.g. via two imports)? (I think this is unlikely, but I'm unfamiliar with the usage patterns of ASTImporterLookupTable, so it'd be easier for you to verify that this kind of failure can't happen.)

Also consider renaming the current `getVaListTagDecl` to e.g. `getOrCreateVaListTagDecl` (it wouldn't blow up the commit size, there are only six references to it in the whole repo) and reusing the name `getVaListTagDecl` for the side-effect-free getter variant. (The `...IfExists` name variant is also acceptable and there are some precedents for it in the codebase, but I think it'd be less surprising on the long term if we reduced the amount of innocent-looking `getSomething` functions that are mutating our state.)

I still think it'd be good extend the patch with the "can we import `int std`" testcase(s), just to be sure. (Although obviously there is no need to run them on the current implementation if you think they'll fail and you already have an alternative solution.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144273/new/

https://reviews.llvm.org/D144273