This is an archive of the discontinued LLVM Phabricator instance.

[clang][DependencyFileGenerator] Fix missing -MT option handling
ClosedPublic

Authored by jkorous on Sep 6 2019, 4:42 PM.

Details

Summary

Some ten years ago the burden of providing target for what later became today's DependencyFileGenerator shifted to Driver in this patch:

52e96cc9329 2009-03-30:  Improve dependency file support.  - Rip out various bits of logic from clang-cc's dependency file gen,    force driver to provide instead.

DependencyFileGenerator later got decoupled from Driver in the sense that it can be used from a program that doesn't even support -MT option (we're using it for example in clang-scan-deps).

I believe we should move the error diagnostics to make sure it's always properly raised - currently it's essentially in one specific method of an implementation detail. It seems there's no reason to not do it right when processing command line arguments.

Also, it seems there's no technical reason why DependencyFileGenerator couldn't work with empty Targets. Afterall it's just used as a prefix for dependency output in void DependencyFileGenerator::outputDependencyFile and it would lead to line starting with the semicolon meant to separate target and it's dependencies this:

: foo.c bar.c baz.c

However, we might still consider that a bug and report the error in some way to clients of DependencyFileGenerator (maybe add some encapsulation and check in constructors?) - I'm open to suggestions.

Diff Detail

Repository
rL LLVM

Event Timeline

jkorous created this revision.Sep 6 2019, 4:42 PM
jkorous edited the summary of this revision. (Show Details)Sep 6 2019, 4:43 PM
jkorous edited the summary of this revision. (Show Details)
jkorous edited the summary of this revision. (Show Details)
arphaman accepted this revision.Sep 6 2019, 5:46 PM

Thanks! I think that's a better layering. LGTM. I don't think there's a particular need to report error to clients of just the DependencyFileGenerator.

This revision is now accepted and ready to land.Sep 6 2019, 5:46 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 6 2019, 5:59 PM