> On Nov 3, 2014, at 2:38 PM, Vassil Vassilev <[email protected]> wrote:
>
> On 03/11/14 16:49, Ben Langmuir wrote:
>>> On Nov 3, 2014, at 2:10 AM, Vassil Vassilev <[email protected]> wrote:
>>>
>>> Hi guys,
>>> I am working on http://llvm.org/bugs/show_bug.cgi?id=20507 Now the
>>> diagnostic gets issued for:
>>> Clang :: Modules/declare-use1.cpp
>>> Clang :: Modules/declare-use2.cpp
>>> Clang :: Modules/declare-use3.cpp
>>> Clang :: Modules/strict-decluse.cpp
>>>
>>> It says smth like: Modules/Inputs/declare-use/module.map:30:10: note:
>>> Header file 'unavailable.h' not present in module 'XF'
>>>
>>> I'd like to add an expected diag to the modulemap file. Eg:
>>> module XF {
>>> ...
>>> header "unavailable.h" // expected-note {{...}}
>>> ...
>>> }
>>>
>>> Is that the right way to go?
>>>
>>> If yes, VerifyDiagnosticConsumer requires some callbacks (such as
>>> HandleComment) which come from the Preprocessor. In the ModuleMapParser we
>>> use raw lexing (without PP at all) and I was wondering what would be the
>>> right way to go, in order to make the -verify flag work inside the module
>>> maps. One solution that I see is to pass the comment handlers from the PP
>>> to the ModuleMapParser, however IMO this would break the encapsulation. Do
>>> you have better ideas?
>> Rather than complicate the module map parsing, we could always put the //
>> expected line into the importing source file (using @file:line). I thought
>> this would Just Work (TM), but when I tried some silly examples the
>> verification didn’t work. I don’t know if that’s related to it being a
>> module map file or something to do with the fatal error. I’m not sure
>> what’s going wrong there, but that’s the direction I would explore.
> Thanks for the pointers. I have prepared a starter already. Wording of the
> diagnostic probably needs rewording :) Do you mind having a look? Thanks!
Sorry for the delay! Some comments:
> Index: include/clang/Basic/DiagnosticCommonKinds.td
> ===================================================================
> --- include/clang/Basic/DiagnosticCommonKinds.td (revision 220824)
> +++ include/clang/Basic/DiagnosticCommonKinds.td (working copy)
> @@ -141,6 +141,7 @@
>
> // Modules
> def err_module_file_conflict : Error<"module '%0' found in both '%1' and
> '%2'”>;
> +def note_module_header_missing : Note<"Header file '%0' not present in
> module '%1'”>;
We don’t capitalize the start of diagnostic messages. I suggest something like:
“header ‘%0’ required by module ‘%1’ not found”
or even
“header ‘%0’ not found"
>
> // TransformActions
> // TODO: Use a custom category name to distinguish rewriter errors.
> Index: lib/Lex/ModuleMap.cpp
> ===================================================================
> --- lib/Lex/ModuleMap.cpp (revision 220824)
> +++ lib/Lex/ModuleMap.cpp (working copy)
> @@ -311,6 +311,15 @@
> }
> }
>
> +void ModuleMap::diagnoseModuleMissingRequirements(Module *M) {
> + assert(M && "Must not be null");
> + for(auto& Header: M->MissingHeaders) {
> + Diags.Report(Header.FileNameLoc, diag::note_module_header_missing)
> + << Header.FileName
> + << M->getFullModuleName();
> + }
> +}
This is using notes, but they need to be attached to an error (maybe you just
haven’t added this yet?). I would have something like:
#include “foo.h” // error: could not import module ‘Foo’
// @ Foo’s module map: note: header ‘bar.h’ required by module ‘Foo’ not found
// @ Foo’s module map: note: header ‘baz.h’ required by module ‘Foo’ not found
As you have already discovered in your test cases below, this is going to
overlap with the “-fmodule-strict-decluse” and “-Wnon-modular-include”
diagnostics when we fall back to the textual include. We need to suppress
those diagnostics when we diagnose a failed import like this, or we need to
make the failed import a fatal error. I would probably go with the former
option, what do you think?
> +
> ModuleMap::KnownHeader
> ModuleMap::findModuleForHeader(const FileEntry *File,
> Module *RequestingModule,
> @@ -331,8 +340,11 @@
> E = Known->second.end();
> I != E; ++I) {
> // Cannot use a module if it is unavailable.
> - if (!I->getModule()->isAvailable())
> + if (!I->getModule()->isAvailable()) {
> + //TODO: Propagate the Loc of the requesting header.
> + diagnoseModuleMissingRequirements(I->getModule()/*, IncludeLoc*/);
> continue;
> + }
>
> // If 'File' is part of 'RequestingModule', 'RequestingModule' is the
> // module we are looking for.
> Index: test/Modules/declare-use2.cpp
> ===================================================================
> --- test/Modules/declare-use2.cpp (revision 220824)
> +++ test/Modules/declare-use2.cpp (working copy)
> @@ -3,5 +3,5 @@
>
> #include "h.h"
> #include "e.h"
> -#include "f.h" // expected-error {{module XH does not depend on a module
> exporting 'f.h'}}
> +#include "f.h" // expected-error {{module XH does not depend on a module
> exporting 'f.h'}} // [email protected]:30 {{Header file
> 'unavailable.h' not present in module 'XF'}}
> const int h2 = h1+e+f;
> Index: test/Modules/Inputs/declare-use/f.h
> ===================================================================
> --- test/Modules/Inputs/declare-use/f.h (revision 220824)
> +++ test/Modules/Inputs/declare-use/f.h (working copy)
> @@ -1,6 +1,6 @@
> #ifndef F_H
> #define F_H
> -#include "a.h"
> -#include "b.h"
> +#include "a.h" // [email protected]:30 {{Header file 'unavailable.h'
> not present in module 'XF'}}
> +#include "b.h" // [email protected]:30 {{Header file 'unavailable.h'
> not present in module 'XF'}}
> const int f = a+b;
> #endif
> Index: test/Modules/Inputs/declare-use/h.h
> ===================================================================
> --- test/Modules/Inputs/declare-use/h.h (revision 220824)
> +++ test/Modules/Inputs/declare-use/h.h (working copy)
> @@ -1,7 +1,7 @@
> #ifndef H_H
> #define H_H
> -#include "c.h"
> -#include "d.h" // expected-error {{does not depend on a module exporting}}
> -#include "h1.h"
> +#include "c.h" // [email protected]:23 {{Header file 'unavailable.h'
> not present in module 'XE'}}
> +#include "d.h" // expected-error {{does not depend on a module exporting}}
> // [email protected]:23 {{Header file 'unavailable.h' not present in
> module 'XE'}}
> +#include "h1.h" // [email protected]:23 {{Header file 'unavailable.h'
> not present in module 'XE'}}
> const int h1 = aux_h*c*7*d;
> #endif
> Index: include/clang/Lex/ModuleMap.h
> ===================================================================
> --- include/clang/Lex/ModuleMap.h (revision 220824)
> +++ include/clang/Lex/ModuleMap.h (working copy)
> @@ -274,6 +274,10 @@
> void diagnoseHeaderInclusion(Module *RequestingModule,
> SourceLocation FilenameLoc, StringRef
> Filename,
> const FileEntry *File);
> + /// \brief Reports issues when a module declaration doesn't match the
> + /// requirements and will be marked as not available.
> + ///
> + void diagnoseModuleMissingRequirements(Module *M);
>
> /// \brief Determine whether the given header is part of a module
> /// marked 'unavailable'.
>
> Vassil
>>
>> Ben
>>
>>> Many thanks,
>>> Vassil
>
>
> --
> --------------------------------------------
> Q: Why is this email five sentences or less?
> A: http://five.sentenc.es
>
> <bug20507.diff>
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
