[PATCH] modularize - header dependencies - version 2

[John Thompson]

Sean,

Thanks for the feedback.

Regarding the usefulness of the feature in general, my opinion is that
modules don't care if headers within the module have dependencies on other
headers within the module.  Perhaps if the headers are in separate modules
it could be problematic.  The user can still choose to fix the dependency
problem by adding #includes in the headers.  Modularize will still help
them find these dependencies by the error generated when the dependencies
are not satisfied.  In this particular case, the set of headers I'm
checking are a platform's standard headers, so I'm trying to minimize the
changes to them, and this change lets me test headers despite the
dependencies, but I explicitly have to specify the needed headers.

However, I'm speaking very subjectively, since I have no idea what the
standard for modules is.  Does anyone have more objective information with
respect to modules?  Doug?

Thanks.

-John



On Fri, Aug 23, 2013 at 11:31 AM, Sean Silva <silvas at purdue.edu> wrote:

>
>   Does this patch make sense from a design standpoint? Aren't these kinds
> of implicit dependencies the things that modularize is trying to get rid of
> in the first place? Why would we expend effort to enshrine them, when we
> can just detect them and emit an error?
>
>   I've also made a few comments about the code, but I think the above
> design issue is more important.
>
>
> ================
> Comment at: modularize/Modularize.cpp:281
> @@ +280,3 @@
> +      std::string File(
> +        std::string("\"") + FileDependents[Index] + std::string("\""));
> +      NewArgs.push_back(FileDependents[Index]);
> ----------------
> Is the quoting here actually necessary? I don't think that there is a
> shell-escaping step after this.
>
> ================
> Comment at: modularize/Modularize.cpp:292-294
> @@ +291,5 @@
> +    SmallVector<const char*, 256> Argv;
> +    for (CommandLineArguments::const_iterator I = CLArgs.begin(),
> +          E = CLArgs.end();
> +        I != E; ++I)
> +      Argv.push_back(I->c_str());
> ----------------
> clang-format
>
> ================
> Comment at: modularize/Modularize.cpp:287-302
> @@ +286,18 @@
> +  // Helper function for finding the input file in an arguments list.
> +  llvm::StringRef findInputFile(const CommandLineArguments &CLArgs) {
> +    OwningPtr<OptTable> Opts(createDriverOptTable());
> +    const unsigned IncludedFlagsBitmask = options::CC1Option;
> +    unsigned MissingArgIndex, MissingArgCount;
> +    SmallVector<const char*, 256> Argv;
> +    for (CommandLineArguments::const_iterator I = CLArgs.begin(),
> +          E = CLArgs.end();
> +        I != E; ++I)
> +      Argv.push_back(I->c_str());
> +    OwningPtr<InputArgList> Args(
> +      Opts->ParseArgs(Argv.data(), Argv.data() + Argv.size(),
> +                      MissingArgIndex, MissingArgCount,
> +                      IncludedFlagsBitmask));
> +    std::vector<std::string> Inputs = Args->getAllArgValues(OPT_INPUT);
> +    return Inputs.back();
> +  }
> +  DependencyMap &Dependencies;
> ----------------
> Does this function actually use any of the class members? It seems like it
> should be a free function.
>
>
> http://llvm-reviews.chandlerc.com/D1474
>



-- 
John Thompson
John.Thompson.JTSoftware at gmail.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130823/038bb96c/attachment.html>