ChuanqiXu accepted this revision. ChuanqiXu added a comment. This revision is now accepted and ready to land.
LGTM except we should remove the dead error emitting. ================ Comment at: clang/lib/Frontend/CompilerInvocation.cpp:2796-2797 + // not intended to be a module map or header unit. + IsHeaderFile = IsHeader && !Preprocessed && !ModuleMap && + HUK == InputKind::HeaderUnit_None; ---------------- iains wrote: > ChuanqiXu wrote: > > Oh, now I find `Header` and `HeaderUnit` might be a little bit confusing. > > It looks like `Header` should include `HeaderUnit` at the first sight. It > > is not true. But I don't have only suggestions... > We have an amount of existing handling specific to "generic headers". > (header unit = none, header = true) > > The intention is that HeaderUnitKind is used to disambiguate the cases we are > dealing with C+=20 header units. > > I am not sure it would be worth the code churn to replace all uses of header > with a HUK. OK, this is a historical problem. It makes no sense require us to fix it in this patch. ================ Comment at: clang/lib/Frontend/CompilerInvocation.cpp:2855-2857 + assert((DashX.getHeaderUnit() == InputKind::HeaderUnit_None || + Inputs.size() == 1) && + "Expected only one input file for header unit"); ---------------- iains wrote: > ChuanqiXu wrote: > > So the compiler would crash if there is multiple inputs for header unit? I > > feel it is not most friendly. How about emitting an error here? > well the driver is supposed to catch these problems (in the case of C++20 > modules mode it will generate several compile jobs, one per header). > > I added an error for this tho. Oh, so if the compiler wouldn't crash in that case, I think the original assertion is acceptable. And the current error is dead code, which is bad. Let's take the original assertion. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121096/new/ https://reviews.llvm.org/D121096 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits