LGTM can you land it?
- Queries
- All Stories
- Search
- Advanced Search
Advanced Search
Jul 15 2019
Okay, that all makes sense to me. Also, in spite of what I said earlier, I think the explicit DUChain locking should be done and there eventually should be some work done to determine which objects will only interact with one specific DUChain per instance of that object, i.e. an object that should reference a specific DUChain, e.g. ClangSupport. We could make the first pass at this involve replacing all of those DUChainReadLocker lock; lines with DUChainReaderLocker lock(DUChain::lock());, but ideally we would eventually come to some understanding about which objects are instantiated within a specific DUChain context such that multiple instances can coexist with entirely independent DUChain interactions. The eventual use case for this is something like T11223 which at this point has so very many unknowns that it would be difficult to start tailoring changes like proposed here to that end. At this stage with the desired changes loosely described above, I am inclined to think that there needs to be some explicitly stated semantics of DUChain::lock(), i.e. it locks all DUChain instances. If we eventually start adding support for multiple DUChain instances we would at least not have to worry about any of these existing locks being overly permissive given the suggested default semantics. I don't know if anything I have just said changes anything about how we would go about applying Clang Thread Safety annotations to this project, but I suspect that it does. Thoughts?
- Added missing header for column with session name
LGTM otherwise
Jul 14 2019
Jul 13 2019
I've tested this in the 5.3 branch now, which required applying hunk #3 manually to texteditor.cpp .
I could try your solution, of course, but what annoys me is that it comes months after I worked on mine. I currently have too many other things going on in my life to dive in and figure out what on earth was going on again. I do think I outlined it well enough above in the initial description and/or exchange; I should find a moment this weekend to sit down and re-read it with a fresh mind.
It would help if you had a specific critique on my solution other than "it doesn't use this or that signal" (or, what I kind of sense, "it comes from you"). No disrespect intended, but your description in D22424 isn't that easy to read either (it felt like reading German, for some inexplicable reason ;) ).
Just a few remarks on the comments that should make them easier to understand (a priori comments should illustrate code and not require lots of different code the understand their meaning ;))
Jul 12 2019
Reasons why currently I favour this over D16882:
- slightly less complex, as it does more what one expects, adding & removing actions on show & hide
- no global static
In D16882#494702, @rjvbb wrote:Please check the earlier discussion; IIRC there is a reliability problem with that signal, and I did try reverting to its use before coming up with the current solution.
Please check the earlier discussion; IIRC there is a reliability problem with that signal, and I did try reverting to its use before coming up with the current solution.
While trying to understand the problem, I found that perhaps we could still revert to using the aboutToHide signal, with proper boundaroes.
So alternative solution proposal up as D22424
Jul 11 2019
Even in contexts where you are specifying the struct itself, it is still matching against the typedef.
In D21589#494068, @Petross404 wrote:In D21589#492408, @sredman wrote:Actually, there is a bit of a problem... The version of the file currently on master is bugged :(
Any progress with the MS tool?
Hm, seeing all this hardcoded code for optional plugins, this calls out for someone to look into making this something pulled in from the language plugins instead :) Not your fault, just mentioning the obvious. So should be fine to just add here.
In D21589#492408, @sredman wrote:Actually, there is a bit of a problem... The version of the file currently on master is bugged :(
In D17908#492404, @sredman wrote:@Petross404 are you still around? Do you have any documentation references for how Microsoft expects the registry to look with regard to which key should contain the Visual Studio installation path? (I don't know if such documentation even exists...)
Jul 10 2019
Remove debugging printf.
Moved to whitelist.
Change to workingDirectory, add comment.
Jul 9 2019
Jul 8 2019
In D22182#492087, @mswan wrote:The workaround being that we eliminate the try-lock behaviour in our locker constructors and move that timeout behaviour elsewhere?
Exactly.
- Fix action boolean.
Actually, a pleasant surprise for today is that this whole mess of registry keys and environment variables is mostly unnecessary as of VS 2017, since Microsoft provides a tool which does all the locating work for us: https://github.com/microsoft/vswhere
Actually, there is a bit of a problem... The version of the file currently on master is bugged :(
Unfortunately, I have a problem with using the batch file from this diff on my system.
I'm okay for the name, thanks a lot for the review and futur commit
In D21589#492284, @kossebau wrote:Ping? Please keep hammering the iron while hot :)
In D16218#361746, @rjvbb wrote:Sh@@t, sorry, I allowed some unrelated (and potential future) changes to pollute this version. Will fix tomorrow.
Ping? Please keep hammering the iron while hot :)
@rjvbb Hi, please Abandon this review, as the reply has been that this should be solved on KDE Frameworks level, in KAboutData (as well as with respective new ECM module for getting the VCS/git revision info into the buildsystem).
@rjvbb Hi, please use the "Abandon this Revision" action, given that this patch/feature was not accepted, so the list of patches to review is not cluttered for everyone.
@tristanp Hi. You do not have push rights for KDE code reposistories, correct? So someone else would need to push for you then. Okay to extend your author name in the commit message to "Tristan Porteries"?
In D22182#492046, @aaronpuchert wrote:I don't find it terribly verbose. Also there are other locks used in the du-chain code, and I think it doesn't hurt to be a little bit more explicit.
Jul 7 2019
In D22182#491716, @mswan wrote:I don't suspect our ..Locker constructors will ever (i.e. not anytime prior to the heat death of the universe) take a different lock. For that reason I am partial to making the lock implicit wherever possible.
That's a bold statement. ;)
In D22182#491022, @aaronpuchert wrote:If we make the lock explicit, we can just annotate the constructor with ACQUIRE[_SHARED](duChainLock) (using the macros from the docs).
Jul 5 2019
In D22158#491427, @kossebau wrote:Not sure yet about all color mappings, that might need some more thinking, at least on a quick test across themes that resulted in quite some unreadable variants. Will play a bit during the upcoming WE.
Interesting approach, might work out, thanks for doing this :)
Jul 4 2019
In D22182#490537, @mswan wrote:I have actually tried changing my experiment with clang's thread safety to use the scoping feature and I seem to be seeing a new issue with these destructors with an error like "lock using shared access, expected exclusive access."
EDIT: I have come across your oldish ticket regarding this: https://bugs.llvm.org/show_bug.cgi?id=33504. Evidently I need to annotate my DUChainReadLocker releasing functions with RELEASE(..) instead of RELEASE_SHARED(..). Not sure I understand why.
I think this DUChain::lock() probably still needs to be exposed as a variable or perhaps it needs to be annotated some way because I suspect that clang is not going to figure out that every value returned from this function is the exact same lock. If I start adding annotations to functions that expect the effectively global DUChain lock to be held beforehand, I would have to write something like REQUIRES(DUChain::lock()) which as I said likely doesn't do what I want.
In D22182#490536, @aaronpuchert wrote:Hmm, you're right. Maybe we can get rid of that when we're certain through static checking that this isn't needed anymore.
Well at least the destructor will need that behaviour because it is certainly sane behaviour for someone to unlock a lock somewhere along the line and then leave it unlocked till the end of the function, e.g. the code I included with this ticket. There didn't seem to be a way that I could scope that lock because this lock was needed for a local constructor invocation.
In D22182#490534, @mswan wrote:I agree, but in the current implementation they are see DUChainReadLocker::lock() and DUChainReadLocker::unlock(), they both check if we are already locked before continuing to lock or unlock respectively.
So it might make sense to try to refactor this DUChain locking abstraction entirely and possibly change all call-sites to respect the new interface. The first obvious question is how do you make this locker thing achieve the same end. It is somewhat convenient that the destructor idempotent behaviour is the way it is. I don't want to invoke lock.lock() at the end of every function to satisfy that destructor and I also wouldn't want to get rid of that destructor behaviour.
In D22182#490533, @aaronpuchert wrote:Lock and unlock shouldn't be idempotent.
In D22182#490532, @mswan wrote:Interesting. I have experimented with this a bit and I cannot find a way to articulate to the thread safety system that my lock and unlock functions are idempotent. If I call lock.unlock() and later return without locking it again, I get a warning (or rather an error because I set -Werror=thread-safety) that complains that I am releasing a lock that isn't held. So would -Wno-error=thread-safety-negative allow us to get around that or is there possibly some other solution?
In D22182#490522, @aaronpuchert wrote:In D22182#489873, @mswan wrote:This should definitely be tried and I would be interested in the task. There is definitely quite alot of stuff that needs to be annotated to make this work so it might need several hands working on it.
I'd be delighted and would happily work with you on this. I worked on the Thread Safety Analysis feature for a while.
The one problem with adopting that system in KDevelop is that we copy DUChain::lock() into each instance of DUChain{Read,Write}Locker which means that our annotations wouldn't see the DUChainReadLocker in one instance as the same lock in another instance.
I don't think the lock is copied, it's just a pointer to it. That's not an issue, std::lock_guard does that as well. Note that DUChainLock would be annotated with __attribute__((capability("mutex"))), but DUChainReadLocker with __attribute__((scoped_lockable)). The analysis tracks which scoped capability owns which capability.
Since I have never seen DUChain{Read,Write}Locker used with anything other than DUChain::lock()
Wondered about this as well—the constructor allows specifying any lock, but this is used only with DUChain::lock().
this shouldn't be cause for false positive deadlock detection.
Deadlock detection needs to turned on separately anyway. (With -Wthread-safety-negative, because it requires "negative capabilities", i.e. someone not having a lock as opposed to having it.)
I wouldn't worry too much about that for now.
In D22182#489873, @mswan wrote:This should definitely be tried and I would be interested in the task. There is definitely quite alot of stuff that needs to be annotated to make this work so it might need several hands working on it.
Jul 3 2019
In D22182#489868, @aaronpuchert wrote:I looked through all of the paths to the failed assertion discussed in that latter ticket and it appears that all but the invocation in PotentialBuddyCollector::accept is verified to have the reader lock held prior to invocation. There were two paths which needed a lock added given my change, so this change set should not cause any regression on bug #386901.
I've always wondered whether Clang's Thread Safety Analysis could be of any help in KDevelop, perhaps it is.
I looked through all of the paths to the failed assertion discussed in that latter ticket and it appears that all but the invocation in PotentialBuddyCollector::accept is verified to have the reader lock held prior to invocation. There were two paths which needed a lock added given my change, so this change set should not cause any regression on bug #386901.
Jul 2 2019
Work with gcc, still have a few failing tests to investigate.
Upload from arc to get everything filled out.
Jul 1 2019
- Rename m_xxxHighlight to xxxHiglight as they are public.
Add now necessary locks to avoid recurrence of bug 386901.
Jun 30 2019
Patch looks good to me. Would you be able to offer screenshots using Breeze?
Please clean the patch of whitespace changes.
Jun 29 2019
Jun 26 2019
Indeed Gitlab should simplify things a lot.
In D21156#486870, @kfunk wrote:Oh, I usually squash my changes into the original commit before uploading the change (i.e. via arc diff). I do not use an extra branch either.
Phew. Anyhow. Let me sum this all up with: Please make your life easier and rather start using KDE's GitLab instance: It's much easier to use since it shares the same workflow as all other major Git hosting platforms out there: https://invent.kde.org/kde/kdevelop
Phabricator is just horrible to adapt to and integrates badly with the usual Git workflow. For KDE development, Phabricator will likely be displaced by GitLab anyway in near future.