This revision was automatically updated to reflect the committed changes.
Closed by commit rL342517: Add a callback for `__has_include` and use it for
dependency scanning. (authored by vsapsai, committed by ).
Herald added subscribers: llvm-commits, jsji.
Changed prior to commit:
dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.
LGTM too.
https://reviews.llvm.org/D30882
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
vsapsai added a comment.
@dexonsmith, does my change address your concerns?
https://reviews.llvm.org/D30882
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
rsmith accepted this revision.
rsmith added inline comments.
Comment at: clang/lib/Frontend/DependencyFile.cpp:340
+return;
+ StringRef Filename = File->getName();
+ if (!FileMatchesDepCriteria(Filename.data(), FileType))
vsapsai wrote:
> rsmith wrote:
> >
vsapsai added inline comments.
Comment at: clang/lib/Frontend/DependencyFile.cpp:300-304
StringRef Filename = FE->getName();
if (!FileMatchesDepCriteria(Filename.data(), FileType))
return;
AddFilename(llvm::sys::path::remove_leading_dotslash(Filename));
rsmith added inline comments.
Comment at: clang/lib/Frontend/DependencyFile.cpp:340
+return;
+ StringRef Filename = File->getName();
+ if (!FileMatchesDepCriteria(Filename.data(), FileType))
Should we really be using the (arbitrary) name of the file from
rsmith added inline comments.
Comment at: lib/Frontend/DependencyFile.cpp:325
+void DFGImpl::HasInclude(SourceLocation Loc, const FileEntry *File) {
+ if (!File)
+return;
vsapsai wrote:
> rsmith wrote:
> > Have you thought about whether we should add a
vsapsai marked 2 inline comments as done.
vsapsai added inline comments.
Comment at: lib/Frontend/DependencyFile.cpp:325
+void DFGImpl::HasInclude(SourceLocation Loc, const FileEntry *File) {
+ if (!File)
+return;
rsmith wrote:
> Have you thought about
vsapsai marked 3 inline comments as done.
vsapsai added inline comments.
Comment at: include/clang/Lex/PPCallbacks.h:263
+ /// \brief Callback invoked when a has_include directive is read.
+ virtual void HasInclude(SourceLocation Loc, const FileEntry *File) {
+ }
vsapsai updated this revision to Diff 165610.
vsapsai added a comment.
- Improve tests, fix -MMD, address comments.
https://reviews.llvm.org/D30882
Files:
clang/include/clang/Lex/PPCallbacks.h
clang/lib/Frontend/DependencyFile.cpp
clang/lib/Lex/PPMacroExpansion.cpp
vsapsai commandeered this revision.
vsapsai added a reviewer: pete.
vsapsai added a comment.
Taking over the change, I'll address the reviewers' comments.
https://reviews.llvm.org/D30882
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
rsmith added inline comments.
Comment at: include/clang/Lex/PPCallbacks.h:263
+ /// \brief Callback invoked when a has_include directive is read.
+ virtual void HasInclude(SourceLocation Loc, const FileEntry *File) {
+ }
This callback seems pretty unhelpful
dexonsmith added a comment.
In https://reviews.llvm.org/D30882#1075589, @dexonsmith wrote:
> In https://reviews.llvm.org/D30882#1075576, @pete wrote:
>
> > Would it be ok to turn this on by default, without a flag, only in the case
> > of the path actually existing, and only the found path
dexonsmith added a comment.
In https://reviews.llvm.org/D30882#1075576, @pete wrote:
> Oh, that actually wasn't my intention when I wrote it.
>
> Honestly I didn't expect it to log anything for missing paths at all, as we
> don't currently log all the missing (but attempted) paths for regular
pete added a comment.
In https://reviews.llvm.org/D30882#1075461, @dexonsmith wrote:
> In https://reviews.llvm.org/D30882#1075407, @ddunbar wrote:
>
> > In https://reviews.llvm.org/D30882#1074822, @dexonsmith wrote:
> >
> > > I don't think this is quite right. I know at least `make`-based
> >
dexonsmith added a comment.
In https://reviews.llvm.org/D30882#1075407, @ddunbar wrote:
> In https://reviews.llvm.org/D30882#1074822, @dexonsmith wrote:
>
> > I don't think this is quite right. I know at least `make`-based
> > incremental builds wouldn't deal well with this.
>
>
> This is
ddunbar added a comment.
In https://reviews.llvm.org/D30882#1074822, @dexonsmith wrote:
> I don't think this is quite right. I know at least `make`-based incremental
> builds wouldn't deal well with this.
This is actually not a novel problem w.r.t. this patch. The exact same
situation comes
dexonsmith requested changes to this revision.
dexonsmith added a comment.
This revision now requires changes to proceed.
Herald added a subscriber: kbarton.
I don't think this is quite right. I know at least `make`-based incremental
builds wouldn't deal well with this.
Given t.cpp:
#if
bruno accepted this revision.
bruno added a comment.
This revision is now accepted and ready to land.
Hi Pete, thanks for working on this!
LGTM with the minor comments below.
Comment at: include/clang/Lex/PPCallbacks.h:264
+ virtual void HasInclude(SourceLocation Loc, const
pete created this revision.
Herald added a subscriber: nemanjai.
This adds a PP callback for the __has_include and __has_include_next directives.
Checking for the presence of a header should add it to the list of header
dependencies so this overrides the callback in the dependency scanner.
I
20 matches
Mail list logo
