yaron.keren added inline comments.
Comment at: clang/lib/Serialization/ASTReader.cpp:4268
// Read the AST block.
if (ASTReadResult Result = ReadASTBlock(F, ClientLoadCapabilities))
+ return Failure;
vsapsai wrote:
> yaron.keren wrote:
> > Result
vsapsai added inline comments.
Comment at: clang/lib/Serialization/ASTReader.cpp:4268
// Read the AST block.
if (ASTReadResult Result = ReadASTBlock(F, ClientLoadCapabilities))
+ return Failure;
yaron.keren wrote:
> Result is unused now.
Thanks
yaron.keren added inline comments.
Comment at: clang/lib/Serialization/ASTReader.cpp:4268
// Read the AST block.
if (ASTReadResult Result = ReadASTBlock(F, ClientLoadCapabilities))
+ return Failure;
Result is unused now.
Comment
This revision was automatically updated to reflect the committed changes.
Closed by commit rG32208555af26: [Modules] Do not remove failed modules after
the control block phase (authored by bnbarham, committed by vsapsai).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
bnbarham updated this revision to Diff 366378.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107690/new/
https://reviews.llvm.org/D107690
Files:
clang/lib/Serialization/ASTReader.cpp
clang/test/VFS/Inputs/UsesFoo.framework/Headers/UsesFoo.h
bnbarham updated this revision to Diff 366154.
bnbarham added a comment.
Forgot to add the braces back in for the case statements
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107690/new/
https://reviews.llvm.org/D107690
Files:
vsapsai accepted this revision.
vsapsai added a comment.
This revision is now accepted and ready to land.
Looks good to me. The only nitpicky thing is I don't remember if the code style
requires braces around multi-line case blocks or not.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE
bnbarham updated this revision to Diff 366152.
bnbarham marked an inline comment as done.
bnbarham edited the summary of this revision.
bnbarham added a comment.
Changed to keep setting the umbrella header/directory with a FIXME that it
currently doesn't work for framework modules.
Repository:
vsapsai added a comment.
Why are you not just changing `return OutOfDate` to `return Failure` for
SUBMODULE_UMBRELLA_HEADER and SUBMODULE_UMBRELLA_DIR? I have no strong opinion
on this but it feels more in line with the rest of the changes. Also I've tried
to do that and nothing seems to be
bnbarham updated this revision to Diff 365619.
bnbarham added a comment.
Removed the now unused UsesFoo.framework in the tests
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107690/new/
https://reviews.llvm.org/D107690
Files:
bnbarham added inline comments.
Comment at: clang/test/VFS/umbrella-mismatch.m:4
-
-// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps
-fmodules-cache-path=%t -ivfsoverlay %t.yaml -F %S/Inputs -fsyntax-only %s
-Wno-atimport-in-framework-header -verify
-// RUN:
vsapsai added a comment.
Made the first review pass and `return Failure` makes sense to me as recovery
isn't the best idea at this point. Still want to check more thoroughly if the
removed code for `SUBMODULE_UMBRELLA_HEADER` and `SUBMODULE_UMBRELLA_DIR` has
any load-bearing side-effects.
bnbarham added a comment.
I have another change to update the post-control-block functions to llvm::Error
instead as well to hopefully make the distinction more clear as well. Let me
know if you think it belongs in this PR, otherwise I'll open another once this
one is in.
Repository:
rG
bnbarham created this revision.
bnbarham added reviewers: vsapsai, dexonsmith.
Herald added a subscriber: kristof.beyls.
bnbarham requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Reading modules first reads each control block in the chain
14 matches
Mail list logo
