Re: [PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-08-04 Thread Puyan Lotfi via cfe-commits
https://reviews.llvm.org/D65704 https://reviews.llvm.org/D65708 Thanks! PL On Thu, Aug 1, 2019 at 5:42 PM Jan Korous wrote: > Oh! Interesting! Thanks for investigating this. > > I'm happy to review the patch. > > On Aug 1, 2019, at 5:17 PM, Puyan Lotfi > wrote: > > Hi Jan, Thanks for the foll

Re: [PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-08-02 Thread Puyan Lotfi via cfe-commits
Hi Jan, Thanks for the follow up! Me and Compnerd narrowed down the issue to the inotify file count limit being exceeded as the cause. DirectoryWatcherLinux::create() in DirectoryWatcher-linux.cpp also doesn't properly perror, I'll post a patch for this shortly to print perror info if the file cou

Re: [PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-08-01 Thread Jan Korous via cfe-commits
Oh! Interesting! Thanks for investigating this. I'm happy to review the patch. > On Aug 1, 2019, at 5:17 PM, Puyan Lotfi wrote: > > Hi Jan, Thanks for the follow up! > > Me and Compnerd narrowed down the issue to the inotify file count limit being > exceeded as the cause. DirectoryWatcherLinu

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-08-01 Thread Jan Korous via Phabricator via cfe-commits
jkorous added a comment. Hi Puyan, I failed to reproduce with llvm.org/master (5faa533e47b0e54b04166b0257c5ebb48e6ffcaa ) on Ubuntu 18.04.1 LTS both in debug and release build. Since it sounds like you can reproduce "reliabl

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-07-31 Thread Jan Korous via Phabricator via cfe-commits
jkorous added a comment. In D58418#1609138 , @plotfi wrote: > @jkorous DirectoryWatcherTests causes ninja check-clang to hang on my Ubuntu > 18.04 computer. check-clang will not finish and I am forced to killall -9 > DirectoryWatcherTests. My system has

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-07-31 Thread Puyan Lotfi via Phabricator via cfe-commits
plotfi added a comment. @jkorous DirectoryWatcherTests causes ninja check-clang to hang on my Ubuntu 18.04 computer. check-clang will not finish and I am forced to killall -9 DirectoryWatcherTests. My system has 40 threads and this repros on ext4 and btrfs. Repository: rL LLVM CHANGES SINC

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-07-09 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added subscribers: beanz, smeenai. smeenai added a comment. In D58418#1577349 , @jkorous wrote: > Thanks for the revert. > > There's actually one more problem - seems like ninja doesn't like the > generated build.ninja file on Linux. > > ninja:

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-07-09 Thread Jan Korous via Phabricator via cfe-commits
jkorous added a comment. Thanks for the revert. There's actually one more problem - seems like ninja doesn't like the generated build.ninja file on Linux. ninja: error: build.ninja:52390: bad $-escape (literal $ must be written as $$) http://lab.llvm.org:8011/builders/clang-with-lto-ubuntu/

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-07-09 Thread Reid Kleckner via Phabricator via cfe-commits
rnk added a comment. Reverted in rC365581 . Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58418/new/ https://reviews.llvm.org/D58418 ___ cfe-commits mailing list cfe-commits@lists.ll

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-07-09 Thread Reid Kleckner via Phabricator via cfe-commits
rnk added inline comments. Comment at: cfe/trunk/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:420 +std::error_code setTimeRes = +llvm::sys::fs::setLastAccessAndModificationTime(FD, NewTimePt, +NewTimePt);

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-07-09 Thread Jan Korous via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rL365574: [clang] DirectoryWatcher (authored by jkorous, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D58418?vs=

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-06-03 Thread Jan Korous via Phabricator via cfe-commits
jkorous updated this revision to Diff 202833. jkorous added a comment. linux implementation - factory method for SemaphorePipe - *_CLOEXEC flags CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58418/new/ https://reviews.llvm.org/D58418 Files: clang/include/clang/DirectoryWatcher/Direc

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-06-03 Thread Jan Korous via Phabricator via cfe-commits
jkorous marked 3 inline comments as done. jkorous added a comment. I fixed the rest. There are still some questions you raised that I just responded to and kept them as not Done. Feel free to take a look. If nothing comes up I'll commit this on Wednesday. Comment at: clang/l

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-05-31 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:317 + + if (pipe(InotifyPollingStopperFDs) == -1) +return nullptr; Use pipe2() with O_CLOEXEC, to avoid leaking the file descriptors to child processes?

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-05-31 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr accepted this revision. gribozavr added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:40 +// FDRead. +// Currently used just for one-off termination signal. +struct SemaphorPipe

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-05-31 Thread Jan Korous via Phabricator via cfe-commits
jkorous updated this revision to Diff 202467. jkorous marked 2 inline comments as done. jkorous added a comment. Addressed comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58418/new/ https://reviews.llvm.org/D58418 Files: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-05-31 Thread Jan Korous via Phabricator via cfe-commits
jkorous marked 20 inline comments as done. jkorous added inline comments. Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:40 +// FDRead. +// Currently used just for one-off termination signal. +struct SemaphorPipe { gribozavr wrote: > grib

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-05-31 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added a comment. Very nice testing approach! Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:20 +/// Provides notifications for file changes in a directory. + +/// Invokes client-provided function on every filesystem event in the watched --

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-05-28 Thread Jan Korous via Phabricator via cfe-commits
jkorous added a comment. One more thing. On macOS FSEvents are coalesced more or less at will and it became quite apparent when I was creating automatic tests - I was for example receiving coalesced events Added & Modified & Removed. We had a discussion about how to deal with this and it turne

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-05-28 Thread Jan Korous via Phabricator via cfe-commits
jkorous updated this revision to Diff 201744. jkorous added a comment. Remove DirectoryWatcher::Event::EventKind::Added CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58418/new/ https://reviews.llvm.org/D58418 Files: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h clang/lib/

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-05-24 Thread Jan Korous via Phabricator via cfe-commits
jkorous updated this revision to Diff 201338. jkorous added a comment. - simplify link libraries in cmake - fix Release build (messed-up asserts) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58418/new/ https://reviews.llvm.org/D58418 Files: clang/include/clang/DirectoryWatcher/Direc

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-05-24 Thread Jan Korous via Phabricator via cfe-commits
jkorous updated this revision to Diff 201321. jkorous added a comment. Specify what "file modified" means and add a test for metadata change CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58418/new/ https://reviews.llvm.org/D58418 Files: clang/cmake/modules/AddClang.cmake clang/incl

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-05-23 Thread Jan Korous via Phabricator via cfe-commits
jkorous marked 6 inline comments as done. jkorous added inline comments. Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:131 + // the async event handling picks them up. Can make this test flaky. + std::this_thread::sleep_for(std::chrono::milliseconds(100)

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-05-23 Thread Jan Korous via Phabricator via cfe-commits
jkorous updated this revision to Diff 201114. jkorous added a comment. Reimplemented tests with std::futures which allowed to use more generous timeout while not slowing down the happy paths. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58418/new/ https://reviews.llvm.org/D58418 File

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-05-22 Thread Jan Korous via Phabricator via cfe-commits
jkorous updated this revision to Diff 200811. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58418/new/ https://reviews.llvm.org/D58418 Files: clang/cmake/modules/AddClang.cmake clang/include/clang/DirectoryWatcher/DirectoryWatcher.h clang/lib/CMakeLists.txt clang/lib/DirectoryWat

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-05-22 Thread Jan Korous via Phabricator via cfe-commits
jkorous updated this revision to Diff 200810. jkorous marked an inline comment as done. jkorous added a comment. Remove busy waits. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58418/new/ https://reviews.llvm.org/D58418 Files: clang/cmake/modules/AddClang.cmake clang/include/clang

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-05-22 Thread Jan Korous via Phabricator via cfe-commits
jkorous marked 9 inline comments as done. jkorous added a comment. Thanks for taking a look Kadir! After yesterday's discussion with Dmitri I removed all those busy waits. Seems like the code is not much more complex now. I am going to update the diff and off to fixing the tests.

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-05-22 Thread Kadir Cetinkaya via Phabricator via cfe-commits
kadircet added inline comments. Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:48 + return llvm::None; +} else { + DirectoryWatcher::Event Front = Events.front(); nit: get rid of the else Comment at: clang

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-05-21 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:136 + +const int PollResult = poll(&PollReq, 1, TimeoutMs); +// There are inotify events waiting to be read! jkorous wrote: > gribozav

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-05-20 Thread Jan Korous via Phabricator via cfe-commits
jkorous updated this revision to Diff 200389. jkorous marked 4 inline comments as done. jkorous added a comment. Addressed comments. Changed semantics of one of std::atomic in linux implementation. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58418/new/ https://reviews.llvm.org/D58418

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-05-20 Thread Jan Korous via Phabricator via cfe-commits
jkorous marked 36 inline comments as done. jkorous added a comment. I addressed most of the comments. What is left: - rewrite test with gmock matchers - write test for metadata of a file in watched dir being changed Comment at: clang/include/clang/DirectoryWatcher/DirectoryWa

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-05-20 Thread Jan Korous via Phabricator via cfe-commits
jkorous marked 2 inline comments as done. jkorous added a comment. Thanks for taking a look @gribozavr! I briefly scanned the rest of your comments and I agree with you (or don't really have a strong opinion) in all cases. I'll work on that today. Comment at: clang/lib/Direct

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-05-20 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:78 + /// The DirectoryWatcher that originated this event is no longer valid and + /// it's behavior is undefined. + /// The prime case is kernel signalling to OS-s

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-05-17 Thread Jan Korous via Phabricator via cfe-commits
jkorous updated this revision to Diff 200123. jkorous added a comment. fix link libraries in cmake CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58418/new/ https://reviews.llvm.org/D58418 Files: clang/cmake/modules/AddClang.cmake clang/include/clang/DirectoryWatcher/DirectoryWatche

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-05-15 Thread Jan Korous via Phabricator via cfe-commits
jkorous updated this revision to Diff 199712. jkorous added a comment. A major clean-up. changelog = **general** - specification, documentation, tests - returning only filenames instead of paths (or empty string when the event is related to the watched dir itself) - removed unsound eve

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-03-21 Thread Jan Korous via Phabricator via cfe-commits
jkorous marked an inline comment as done. jkorous added a comment. In D58418#1431765 , @thakis wrote: > In D58418#1431399 , @jkorous wrote: > > > In D58418#1430630 , @thakis

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-03-15 Thread Nico Weber via Phabricator via cfe-commits
thakis added a comment. In D58418#1431399 , @jkorous wrote: > In D58418#1430630 , @thakis wrote: > > > In D58418#1430490 , @jkorous wrote: > > > > > In D58418#1430160

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-03-15 Thread Jan Korous via Phabricator via cfe-commits
jkorous added a comment. In D58418#1430630 , @thakis wrote: > In D58418#1430490 , @jkorous wrote: > > > In D58418#1430160 , @thakis wrote: > > > > > Why is this needed for in

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-03-15 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr requested changes to this revision. gribozavr added inline comments. This revision now requires changes to proceed. Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:27 +/// be invoked even if the directory is empty. +class DirectoryWatcher { +public:

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-03-15 Thread Nico Weber via Phabricator via cfe-commits
thakis added a comment. In D58418#1430490 , @jkorous wrote: > In D58418#1430160 , @thakis wrote: > > > Why is this needed for index-while-building? My mental model for > > index-while-building is that that streams

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-03-15 Thread Jan Korous via Phabricator via cfe-commits
jkorous added a comment. In D58418#1430160 , @thakis wrote: > Why is this needed for index-while-building? My mental model for > index-while-building is that that streams out build index metadata as part of > the regular compile. Why does that require wa

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-03-14 Thread Nico Weber via Phabricator via cfe-commits
thakis added a comment. Why is this needed for index-while-building? My mental model for index-while-building is that that streams out build index metadata as part of the regular compile. Why does that require watching directories? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58418/ne

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-03-14 Thread Jan Korous via Phabricator via cfe-commits
jkorous updated this revision to Diff 190744. jkorous added a comment. Remove duplicate comment CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58418/new/ https://reviews.llvm.org/D58418 Files: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h clang/lib/CMakeLists.txt clang/l

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-03-12 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added a comment. Sorry for the delay. I will finish reviewing tomorrow. Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:9 +/// \file +/// \brief Utility class for listening for file system changes in a directory. +//===

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-03-12 Thread Jan Korous via Phabricator via cfe-commits
jkorous added a comment. Ping. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58418/new/ https://reviews.llvm.org/D58418 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-03-04 Thread Jan Korous via Phabricator via cfe-commits
jkorous added reviewers: kadircet, gribozavr. jkorous added a comment. Adding clangd folks in case they want to take a look. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58418/new/ https://reviews.llvm.org/D58418 ___ cfe-commits mailing li

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-02-28 Thread Jan Korous via Phabricator via cfe-commits
jkorous updated this revision to Diff 188808. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58418/new/ https://reviews.llvm.org/D58418 Files: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h clang/lib/CMakeLists.txt clang/lib/DirectoryWatcher/CMakeLists.txt clang/lib/Direc

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-02-28 Thread Jan Korous via Phabricator via cfe-commits
jkorous updated this revision to Diff 188783. jkorous added a comment. - fix the linux implementation - clang-format CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58418/new/ https://reviews.llvm.org/D58418 Files: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h clang/lib/CMa

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-02-28 Thread David Zarzycki via Phabricator via cfe-commits
davezarzycki added a comment. Can we please revert this code? One cannot use dynamic_cast on a char *. Did you mean reinterpret_cast? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58418/new/ https://reviews.llvm.org/D58418 ___ cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-02-27 Thread Jan Korous via Phabricator via cfe-commits
jkorous added a comment. @xbolva00 I prefer to keep it here for now. I suggest we wait with upstreaming until an actual need for use in some other project arises. This is what we basically did with VFS. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58418/new/ https://reviews.llvm.org/

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-02-27 Thread Dávid Bolvanský via Phabricator via cfe-commits
xbolva00 added a comment. Maybe move it to LLVM? As this watcher may be useful for other projects in the future? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58418/new/ https://reviews.llvm.org/D58418 ___ cfe-commits mailing list cfe-comm

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-02-27 Thread Jan Korous via Phabricator via cfe-commits
jkorous updated this revision to Diff 188586. jkorous added a comment. - moved checks for CoreServices/inotify to cmake CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58418/new/ https://reviews.llvm.org/D58418 Files: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h clang/lib/

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-02-26 Thread Jan Korous via Phabricator via cfe-commits
jkorous updated this revision to Diff 188423. jkorous marked 4 inline comments as done. jkorous added a comment. - Use RetryAfterSignal - Propagate events from inotify directly - Remove ModTime - Add assert for unknown event kinds CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58418/new/

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-02-26 Thread Jan Korous via Phabricator via cfe-commits
jkorous marked 18 inline comments as done. jkorous added inline comments. Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:116 + + DirectoryWatcher::EventKind K = DirectoryWatcher::EventKind::Added; + if (ievt->mask & IN_MODIFY) { jko

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-02-23 Thread Argyrios Kyrtzidis via Phabricator via cfe-commits
akyrtzi added inline comments. Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:135 +if (!statusOpt.hasValue()) + K = DirectoryWatcher::EventKind::Removed; + } mgorny wrote: > akyrtzi wrote: > > mgorny wrote: > > > akyrtzi

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-02-23 Thread Michał Górny via Phabricator via cfe-commits
mgorny added inline comments. Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:135 +if (!statusOpt.hasValue()) + K = DirectoryWatcher::EventKind::Removed; + } akyrtzi wrote: > mgorny wrote: > > akyrtzi wrote: > > > mgorny

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-02-23 Thread Argyrios Kyrtzidis via Phabricator via cfe-commits
akyrtzi added inline comments. Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:135 +if (!statusOpt.hasValue()) + K = DirectoryWatcher::EventKind::Removed; + } mgorny wrote: > akyrtzi wrote: > > mgorny wrote: > > > akyrtzi

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-02-23 Thread Michał Górny via Phabricator via cfe-commits
mgorny added inline comments. Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:135 +if (!statusOpt.hasValue()) + K = DirectoryWatcher::EventKind::Removed; + } akyrtzi wrote: > mgorny wrote: > > akyrtzi wrote: > > > mgorny

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-02-23 Thread Argyrios Kyrtzidis via Phabricator via cfe-commits
akyrtzi added inline comments. Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:135 +if (!statusOpt.hasValue()) + K = DirectoryWatcher::EventKind::Removed; + } mgorny wrote: > akyrtzi wrote: > > mgorny wrote: > > > jkorous

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-02-23 Thread Michał Górny via Phabricator via cfe-commits
mgorny added inline comments. Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:135 +if (!statusOpt.hasValue()) + K = DirectoryWatcher::EventKind::Removed; + } akyrtzi wrote: > mgorny wrote: > > jkorous wrote: > > > mgorny

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-02-23 Thread Argyrios Kyrtzidis via Phabricator via cfe-commits
akyrtzi added inline comments. Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:135 +if (!statusOpt.hasValue()) + K = DirectoryWatcher::EventKind::Removed; + } mgorny wrote: > jkorous wrote: > > mgorny wrote: > > > Why? I

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-02-22 Thread Michał Górny via Phabricator via cfe-commits
mgorny added inline comments. Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:196 + while (true) { +if (close(inotifyFD) == -1 && errno == EINTR) + continue; There's some fancy function for this in LLVM. `RetryAfterSignal`, I think.

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-02-22 Thread Duncan P. N. Exon Smith via Phabricator via cfe-commits
dexonsmith added inline comments. Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:154 +errorMsg += llvm::sys::StrError(); +return true; + }; jkorous wrote: > mgorny wrote: > > The return values seem to be confusing. Any reason not to

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-02-22 Thread Jan Korous via Phabricator via cfe-commits
jkorous marked 4 inline comments as done. jkorous added inline comments. Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:116 + + DirectoryWatcher::EventKind K = DirectoryWatcher::EventKind::Added; + if (ievt->mask & IN_MODIFY) { mgor

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-02-21 Thread Jan Korous via Phabricator via cfe-commits
jkorous marked an inline comment as done. jkorous added a comment. Fixed closing of FD. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58418/new/ https://reviews.llvm.org/D58418 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https:/

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-02-21 Thread Jan Korous via Phabricator via cfe-commits
jkorous updated this revision to Diff 187868. jkorous added a comment. - handle errors returned by close CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58418/new/ https://reviews.llvm.org/D58418 Files: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h clang/lib/CMakeLists.txt

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-02-21 Thread Jan Korous via Phabricator via cfe-commits
jkorous added a comment. Thanks for taking a look Michał! I'll try to address the rest of your comments asap. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58418/new/ https://reviews.llvm.org/D58418 ___ cfe-commits mailing list cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-02-21 Thread Jan Korous via Phabricator via cfe-commits
jkorous updated this revision to Diff 187860. jkorous marked 2 inline comments as done. jkorous added a comment. - handle EINTR - assert we are reading whole INotifyEvents CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58418/new/ https://reviews.llvm.org/D58418 Files: clang/include/cl

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-02-21 Thread Jan Korous via Phabricator via cfe-commits
jkorous updated this revision to Diff 187850. jkorous added a comment. - license - end-of-include-guard comments CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58418/new/ https://reviews.llvm.org/D58418 Files: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h clang/lib/CMakeLi

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-02-20 Thread Michał Górny via Phabricator via cfe-commits
mgorny added a comment. Also, don't forget that `inotify()` is not 100% reliable on Linux, and it can miss events under high loads. So technically you should probably include periodic directory scans if you are going to rely on this actually reporting every file added. Comme

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-02-20 Thread Jan Korous via Phabricator via cfe-commits
jkorous updated this revision to Diff 187623. jkorous added a comment. [DirectoryWatcher] Fix detection of FSEvents for iOS [DirectoryWatcher][NFC] Doxygen CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58418/new/ https://reviews.llvm.org/D58418 Files: clang/include/clang/DirectoryWat

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-02-19 Thread Jan Korous via Phabricator via cfe-commits
jkorous created this revision. jkorous added reviewers: arphaman, dexonsmith, akyrtzi, nathawes, yvvan. Herald added subscribers: cfe-commits, jdoerfert, jfb, mgorny. Herald added a project: clang. This patch contains implementation of DirectoryWatcher from github.com/apple/swift-clang We are st