[PATCH] D70351: [clang][WIP][clang-scan-deps] Add an experimental C API.

kousikk added a comment. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70351/new/ https://reviews.llvm.org/D70351 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/

[PATCH] D70351: [clang][WIP][clang-scan-deps] Add an experimental C API.

kousikk added a comment. @Bigcheese wondering if there are things with respect to testing we can do, to help the patch move forward :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70351/new/ https://reviews.llvm.org/D70351

[PATCH] D69122: Add support to find out resource dir and add it as compilation args

This revision was automatically updated to reflect the committed changes. Closed by commit rG26fa9e31f58a: Add support to find out resource dir and add it as compilation args (authored by kousikk). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69122/

[PATCH] D70351: [clang][WIP][clang-scan-deps] Add an experimental C API.

kousikk added inline comments. Comment at: clang/include/clang-c/Dependencies.h:146 + */ +typedef struct CXOpaqueDependencyScannerWorker *CXDependencyScannerWorker; + It would be simpler if the clients didn't have to worry about the worker? As far as a user of th

[PATCH] D69122: Add support to find out resource dir and add it as compilation args

kousikk added inline comments. Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:75-79 +llvm::Optional Redirects[] = { +{""}, // Stdin +StringRef(OutputFile), +StringRef(ErrorFile), +}; Bigcheese wrote: > It would be nice i

[PATCH] D69122: Add support to find out resource dir and add it as compilation args

kousikk updated this revision to Diff 230508. kousikk marked 4 inline comments as done. kousikk added a comment. Address review comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69122/new/ https://reviews.llvm.org/D69122 Files: clang/tools/

[PATCH] D69122: Add support to find out resource dir and add it as compilation args

kousikk added a comment. Ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69122/new/ https://reviews.llvm.org/D69122 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi

[PATCH] D69122: Add support to find out resource dir and add it as compilation args

kousikk updated this revision to Diff 227601. kousikk added a comment. Forgot to run clang-format Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69122/new/ https://reviews.llvm.org/D69122 Files: clang/tools/clang-scan-deps/ClangScanDeps.cpp Inde

[PATCH] D69122: Add support to find out resource dir and add it as compilation args

kousikk updated this revision to Diff 227600. kousikk marked an inline comment as done. kousikk added a comment. @BigCheese - thanks, all the three points make sense! > An alternative would be to only do FindResourceDir when given an absolute > path to a compiler, instead of trying to guess. I t

[PATCH] D69122: Add support to find out resource dir and add it as compilation args

kousikk added a comment. I discussed with @klimek about this. The conclusion we arrived at was: Allowing override of `resource-dir` for all tools is not something we should do (for reasons that @klimek has outlined above). However, the dependency scanner tool can choose to override `resource-di

[PATCH] D69420: [clang][clang-scan-deps] Add support for extracting full module dependencies.

kousikk added a comment. A few nit comments Comment at: clang/include/clang/Tooling/DependencyScanning/DependencyScanningService.h:34 +/// The format that is output by the dependency scanner. +enum class ScanningFormat { + /// This is the Makefile compatible dep format. -

[PATCH] D69186: Refactor DependencyScanningTool to its own file

This revision was automatically updated to reflect the committed changes. Closed by commit rGfb042b094fda: Refactor DependencyScanningTool to its own file (authored by kousikk). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69186/new/ https://review

[PATCH] D69186: Refactor DependencyScanningTool to its own file

kousikk updated this revision to Diff 225994. kousikk added a comment. Run clang format Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69186/new/ https://reviews.llvm.org/D69186 Files: clang/include/clang/Tooling/DependencyScanning/DependencyScan

[PATCH] D69090: [Try 2] Include sanitize blacklist and other extra deps as part of scan-deps output

kousikk added a comment. I was able to test this on a Windows machine. The `ExtraDeps` are already being added to the dependency output (this code does that - https://github.com/llvm/llvm-project/blob/master/clang/lib/Frontend/DependencyFile.cpp#L188). My suspicion for why I wasn't seeing them

[PATCH] D69186: Refactor DependencyScanningTool to its own file

kousikk created this revision. kousikk added reviewers: arphaman, jkorous, Bigcheese, dexonsmith. Herald added subscribers: cfe-commits, mgorny. Herald added a project: clang. kousikk updated this revision to Diff 225667. kousikk added a comment. Lint fixes There's no behavior change - just movi

[PATCH] D69186: Refactor DependencyScanningTool to its own file

kousikk updated this revision to Diff 225667. kousikk added a comment. Lint fixes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69186/new/ https://reviews.llvm.org/D69186 Files: clang/include/clang/Tooling/DependencyScanning/DependencyScanningTo

[PATCH] D69090: [Try 2] Include sanitize blacklist and other extra deps as part of scan-deps output

kousikk added a comment. Thanks for the comment @jkorous. > I think you could've just used CHECK-DAG to fix the tests. It *might* be a > bit more robust. Although just reordering checks seems perfectly fine too. > https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-dag-directive. > Usi

[PATCH] D69122: Add support to find out resource dir and add it as compilation args

kousikk added a comment. > I haven't looked into this in detail but it feels kind of wasteful to start a > process just to get this value. Right, that's why I cache this information. So multiple compile commands sharing the same compiler process will trigger at 1 subprocess and then subsequent

[PATCH] D69122: Add support to find out resource dir and add it as compilation args

kousikk created this revision. kousikk added reviewers: arphaman, Bigcheese, jkorous, dexonsmith. Herald added a project: clang. Herald added a subscriber: cfe-commits. kousikk edited the summary of this revision. kousikk edited the summary of this revision. If -resource-dir is not specified as pa

[PATCH] D69090: [Try 2] Include sanitize blacklist and other extra deps as part of scan-deps output

kousikk updated this revision to Diff 225375. kousikk added a comment. Fix tests now that output is in ascending order Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69090/new/ https://reviews.llvm.org/D69090 Files: clang/lib/Tooling/DependencySc

[PATCH] D69090: [Try 2] Include sanitize blacklist and other extra deps as part of scan-deps output

kousikk created this revision. kousikk added reviewers: arphaman, dexonsmith, Bigcheese, jkorous. kousikk added a project: clang. kousikk edited the summary of this revision. Clang's -M mode includes these extra dependencies in its output and clang-scan-deps should have equivalent behavior, so ad

[PATCH] D69079: Revert "Include sanitize blacklist and other extra deps as part of scan-deps output"This test is failing on Windows bots, revert for now (will check the right fix and retry the patch).

This revision was automatically updated to reflect the committed changes. Closed by commit rG9e7e36d4c260: Revert "Include sanitize blacklist and other extra deps as part of scan-deps… (authored by kousikk). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.o

[PATCH] D69079: Revert "Include sanitize blacklist and other extra deps as part of scan-deps output"This test is failing on Windows bots, revert for now (will check the right fix and retry the patch).

kousikk created this revision. kousikk added reviewers: Bigcheese, jkorous, arphaman. Herald added subscribers: cfe-commits, dexonsmith. Herald added a project: clang. kousikk added a comment. Failure - http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/11583 This reverts commit 962

[PATCH] D69079: Revert "Include sanitize blacklist and other extra deps as part of scan-deps output"This test is failing on Windows bots, revert for now (will check the right fix and retry the patch).

kousikk added a comment. Failure - http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/11583 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69079/new/ https://reviews.llvm.org/D69079 ___ cfe

[PATCH] D69017: Include sanitize blacklist and other extra deps as part of scan-deps output

This revision was automatically updated to reflect the committed changes. Closed by commit rG962ca076e51c: Include sanitize blacklist and other extra deps as part of scan-deps output (authored by kousikk). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org

[PATCH] D69017: Include sanitize blacklist and other extra deps as part of scan-deps output

kousikk updated this revision to Diff 225153. kousikk added a comment. Lint fixes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69017/new/ https://reviews.llvm.org/D69017 Files: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp

[PATCH] D69017: Include sanitize blacklist and other extra deps as part of scan-deps output

kousikk created this revision. kousikk added reviewers: arphaman, dexonsmith, Bigcheese, jkorous. Herald added a project: clang. Herald added a subscriber: cfe-commits. kousikk updated this revision to Diff 225153. kousikk added a comment. Lint fixes Clang's -M mode includes these extra dependen

[PATCH] D68193: In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

This revision was automatically updated to reflect the committed changes. Closed by commit rG4abac5330277: In openFileForRead don't cache erroneous entries if the error relates to them… (authored by kousikk). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.

[PATCH] D68193: In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

kousikk updated this revision to Diff 224228. kousikk added a comment. Remove getError() function that's now unused Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68193/new/ https://reviews.llvm.org/D68193 Files: clang/include/clang/Tooling/Depen

[PATCH] D68193: In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

kousikk updated this revision to Diff 224219. kousikk added a comment. - Revert changes to createFileEntry since that may result in a race condition if stat is called before file open Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68193/new/ https:

[PATCH] D68193: In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

kousikk added a comment. @arphaman I don't mind changing this if there are race conditions as you say, but isn't the assumption of the tool that the filesystem remains unchanged for a single run of the tool? If so, should we actually throw error conditions instead of crashing in those cases?

[PATCH] D68193: In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

kousikk added a comment. @dexonsmith when you get a chance, can you take another look at this? Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68193/new/ https://reviews.llvm.org/D68193 ___ cfe-c

[PATCH] D68436: [clang-scan-deps] Improve string/character literal skipping

kousikk accepted this revision. kousikk marked an inline comment as done. kousikk added inline comments. Comment at: clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp:206 +if (*First == '\\') { + if (++First == End) +return; dexonsmith wrote:

[PATCH] D68436: [clang-scan-deps] Improve string/character literal skipping

kousikk added inline comments. Comment at: clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp:205 + return; +if (*First == '\\') { + if (++First == End) Should you also check if the character right after a backslash is equal to Terminator and if

[PATCH] D68193: In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

kousikk updated this revision to Diff 222953. kousikk added a comment. Update diff to include all commits Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68193/new/ https://reviews.llvm.org/D68193 Files: clang/include/clang/Tooling/DependencyScann

[PATCH] D68193: In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

kousikk added a comment. In D68193#1692293 , @arphaman wrote: > I don't quite understand the specific issue you're hitting, as it sounds that > the logic right now should handle it: > > > It seems that when the CachingFileSystem is first given a file to o

[PATCH] D68193: In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

kousikk updated this revision to Diff 222669. kousikk added a comment. Add missed const Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68193/new/ https://reviews.llvm.org/D68193 Files: clang/include/clang/Tooling/DependencyScanning/DependencyScan

[PATCH] D68193: In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

kousikk added a comment. In D68193#1688838 , @dexonsmith wrote: > Sorry for bouncing you around, but I just had a look at the other user of > `createFileEntry` and I think the right thing to do is to somehow share the > code between `DependencyScanningWo

[PATCH] D68193: In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

kousikk updated this revision to Diff 222667. kousikk marked an inline comment as done. kousikk added a comment. Address code-reorg comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68193/new/ https://reviews.llvm.org/D68193 Files: clang/inc

[PATCH] D68193: In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

kousikk marked 3 inline comments as done. kousikk added inline comments. Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:236-237 if (!CacheEntry.isValid()) { + llvm::vfs::FileSystem &FS = getUnderlyingFS(); + auto MaybeStatus = FS

[PATCH] D68193: In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

kousikk updated this revision to Diff 222474. kousikk added a comment. Avoid the extra stat() call Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68193/new/ https://reviews.llvm.org/D68193 Files: clang/include/clang/Tooling/DependencyScanning/Dep

[PATCH] D68193: In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

kousikk created this revision. kousikk added a reviewer: arphaman. Herald added subscribers: cfe-commits, dexonsmith. Herald added a project: clang. It seems that when the CachingFileSystem is first given a file to open that is actually a directory, it incorrectly caches that path to be errenous

[PATCH] D67635: Fix for stringized function-macro args continued across lines

kousikk added a comment. > I think you should obtain commit access for your future patches/commits. You > can follow the instructions here: > https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access Thanks, I've done that! Repository: rL LLVM CHANGES SINCE LAST ACTION https://r

[PATCH] D67635: Fix for stringized function-macro args continued across lines

kousikk added a comment. Thanks! I will need you to merge this one too! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67635/new/ https://reviews.llvm.org/D67635 ___ cfe-commits mailing list cfe-commits

[PATCH] D67635: Fix for stringized function-macro args continued across lines

kousikk added a comment. Ping on the review for this :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67635/new/ https://reviews.llvm.org/D67635 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D67635: Fix for stringized function-macro args continued across lines

kousikk created this revision. kousikk added a reviewer: arphaman. Herald added subscribers: cfe-commits, dexonsmith. Herald added a project: clang. In case of certain #define'd macros, there's a space just before line continuation that the minimized-source lexer was missing to include, resulting

[PATCH] D67091: Fix for headers having the same name as a directory

kousikk added a comment. I don't have commit access - do you mind submitting this for me? Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67091/new/ https://reviews.llvm.org/D67091 ___ cfe-commit

[PATCH] D67091: Fix for headers having the same name as a directory

kousikk added a comment. > I was unable to reproduce the failures when I moved the check to createFile. > What kind of failures did you see? Did you move it right to the start of the > function? I would recommend rebasing the patch and retrying. I saw an assertion error - but I realize now that

[PATCH] D67091: Fix for headers having the same name as a directory

kousikk updated this revision to Diff 219976. kousikk added a comment. - Add validation inside the createFile() function Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67091/new/ https://reviews.llvm.org/D67091 Files: clang/include/clang/Tooling/

[PATCH] D67091: Fix for headers having the same name as a directory

kousikk updated this revision to Diff 219975. kousikk added a comment. - Add validation inside the createFile() function Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67091/new/ https://reviews.llvm.org/D67091 Files: clang/include/clang/Tooling/

[PATCH] D67091: Fix for headers having the same name as a directory

kousikk added a comment. Sorry about the delay on this - I was OOO (back now). 1. I added tests. 2. I couldn't add isDirectory() check to createFile() since that resulted in failures to normal scenarios where it was previously passing. PTAL! Repository: rG LLVM Github Monorepo CHANGES SINC

[PATCH] D67091: Fix for headers having the same name as a directory

kousikk updated this revision to Diff 219918. kousikk added a comment. - Add validation inside the createFile() function Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67091/new/ https://reviews.llvm.org/D67091 Files: clang/include/clang/Tooling/

[PATCH] D67091: Fix for headers having the same name as a directory

kousikk updated this revision to Diff 219917. kousikk added a comment. - Add tests and remove the fix inside createFile since it fails in other cases Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67091/new/ https://reviews.llvm.org/D67091 Files:

[PATCH] D67091: Fix for headers having the same name as a directory

kousikk updated this revision to Diff 219890. kousikk added a comment. - Add validation inside the createFile() function Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67091/new/ https://reviews.llvm.org/D67091 Files: clang/include/clang/Tooling/

[PATCH] D67091: Fix for headers having the same name as a directory

kousikk updated this revision to Diff 219891. kousikk added a comment. - Add validation inside the createFile() function Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67091/new/ https://reviews.llvm.org/D67091 Files: clang/include/clang/Tooling/

[PATCH] D67091: Fix for headers having the same name as a directory

kousikk added a comment. Sure I'll add a test case! Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:196 + return llvm::ErrorOr>(std::make_error_code(std::errc::is_a_directory)); +} + } arphaman wrote: > This change

[PATCH] D67091: Fix for headers having the same name as a directory

kousikk created this revision. kousikk added a reviewer: arphaman. Herald added subscribers: cfe-commits, jfb, dexonsmith. Herald added a project: clang. Scan deps tool crashes when called on a C++ file, containing an include that has the same name as a directory. For example: test.cpp: #include