Feed Advanced Search

Yesterday

kossebau updated the diff for D22424: TextDocument: remove actions from contextmenu on hide already.

update to first review feedback

Mon, Jul 15, 9:33 PM · KDevelop
kossebau added a comment to D22424: TextDocument: remove actions from contextmenu on hide already.

Thanks for first review & testing.

Mon, Jul 15, 9:33 PM · KDevelop
mdlubakowski added a comment to D22456: Show session name in the Delete Session confirmation dialog.
In D22456#496025, @apol wrote:

LGTM can you land it?

Mon, Jul 15, 7:33 PM · KDevelop
apol accepted D22456: Show session name in the Delete Session confirmation dialog.

LGTM can you land it?

Mon, Jul 15, 6:41 PM · KDevelop
mswan added a comment to D22182: Remove anchorClicked lock which causes deadlock.

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 have 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 with 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?

Mon, Jul 15, 2:00 PM · KDevelop
mswan updated the task description for T11223: Option to support target-specific DUChain.
Mon, Jul 15, 1:45 PM · KDevelop
mswan updated the task description for T11223: Option to support target-specific DUChain.
Mon, Jul 15, 1:32 PM · KDevelop
mswan triaged T11224: Build target context view as Wishlist priority.
Mon, Jul 15, 1:19 PM · KDevelop
mswan updated the task description for T11223: Option to support target-specific DUChain.
Mon, Jul 15, 1:07 PM · KDevelop
mswan triaged T11223: Option to support target-specific DUChain as Wishlist priority.
Mon, Jul 15, 12:58 PM · KDevelop
mdlubakowski updated the diff for D22456: Show session name in the Delete Session confirmation dialog.
  • Added missing header for column with session name
Mon, Jul 15, 5:25 AM · KDevelop
apol added a comment to D22456: Show session name in the Delete Session confirmation dialog.

LGTM otherwise

Mon, Jul 15, 1:16 AM · KDevelop
apol accepted D22455: Fix code completion for nameless structs/unions with the same member.
Mon, Jul 15, 1:14 AM · KDevelop

Sun, Jul 14

mdlubakowski added a reviewer for D22456: Show session name in the Delete Session confirmation dialog: KDevelop.
Sun, Jul 14, 5:09 PM · KDevelop
mdlubakowski retitled D22456: Show session name in the Delete Session confirmation dialog from Show session name in the Delete Session confimration dialog to Show session name in the Delete Session confirmation dialog.
Sun, Jul 14, 5:07 PM · KDevelop
mdlubakowski requested review of D22456: Show session name in the Delete Session confirmation dialog.
Sun, Jul 14, 5:04 PM · KDevelop
buschinski requested review of D22455: Fix code completion for nameless structs/unions with the same member.
Sun, Jul 14, 2:43 PM · KDevelop

Sat, Jul 13

davidre updated the task description for T11221: Grepview - Group matches by line.
Sat, Jul 13, 7:06 PM · KDevelop
davidre created T11221: Grepview - Group matches by line.
Sat, Jul 13, 7:04 PM · KDevelop
rjvbb added a comment to D22424: TextDocument: remove actions from contextmenu on hide already.

I've tested this in the 5.3 branch now, which required applying hunk #3 manually to texteditor.cpp .

Sat, Jul 13, 3:36 PM · KDevelop
rjvbb added inline comments to D22424: TextDocument: remove actions from contextmenu on hide already.
Sat, Jul 13, 1:11 PM · KDevelop
anthonyfieroni added inline comments to D22424: TextDocument: remove actions from contextmenu on hide already.
Sat, Jul 13, 10:59 AM · KDevelop
rjvbb added a comment to D16882: [KDevelop/Shell] prevent duplicate added contextmenu actions.

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 ;) ).

Sat, Jul 13, 9:51 AM · KDevelop
rjvbb added a comment to D22424: TextDocument: remove actions from contextmenu on hide already.

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 ;))

Sat, Jul 13, 9:51 AM · KDevelop

Fri, Jul 12

kossebau added a comment to D22424: TextDocument: remove actions from contextmenu on hide already.

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
Fri, Jul 12, 4:21 PM · KDevelop
kossebau added a comment to D16882: [KDevelop/Shell] prevent duplicate added contextmenu actions.

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.

Fri, Jul 12, 3:39 PM · KDevelop
rjvbb added a comment to D16882: [KDevelop/Shell] prevent duplicate added contextmenu actions.

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.

Fri, Jul 12, 3:32 PM · KDevelop
kossebau added a comment to D16882: [KDevelop/Shell] prevent duplicate added contextmenu actions.

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

Fri, Jul 12, 3:13 PM · KDevelop
kossebau requested review of D22424: TextDocument: remove actions from contextmenu on hide already.
Fri, Jul 12, 3:10 PM · KDevelop

Thu, Jul 11

mswan updated the task description for T11209: Determine better policy for handling ahead-of-time struct definitions.
Thu, Jul 11, 10:25 PM · KDevelop
mswan added a comment to T11209: Determine better policy for handling ahead-of-time struct definitions.

Even in contexts where you are specifying the struct itself, it is still matching against the typedef.

Thu, Jul 11, 9:32 PM · KDevelop
sredman added a comment to D21589: Duplicate Visual Studio 2017 hack for VS 2019.

Actually, there is a bit of a problem... The version of the file currently on master is bugged :(

See: https://phabricator.kde.org/D17908#492404

Any progress with the MS tool?

Thu, Jul 11, 9:21 PM · KDevelop
kossebau added a comment to D22062: Addition of php script output patterns. Initial development..

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.

Thu, Jul 11, 2:41 PM · KDevelop
kossebau added reviewers for D22062: Addition of php script output patterns. Initial development.: KDevelop, pprkut.
Thu, Jul 11, 2:33 PM · KDevelop
Petross404 added a comment to D21589: Duplicate Visual Studio 2017 hack for VS 2019.

Actually, there is a bit of a problem... The version of the file currently on master is bugged :(

See: https://phabricator.kde.org/D17908#492404

Thu, Jul 11, 9:56 AM · KDevelop
Petross404 added a comment to D17908: kdevelop-msvc.bat finds VS-2017 based on a registry key on Windows..

@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...)

Thu, Jul 11, 9:55 AM · KDevelop
mswan triaged T11209: Determine better policy for handling ahead-of-time struct definitions as Normal priority.
Thu, Jul 11, 7:30 AM · KDevelop

Wed, Jul 10

bungeman updated the summary of D22217: Pass extra build flags to compiler for parsing..
Wed, Jul 10, 10:48 PM · KDevelop
bungeman updated the diff for D22217: Pass extra build flags to compiler for parsing..

Remove debugging printf.

Wed, Jul 10, 10:47 PM · KDevelop
bungeman updated the diff for D22217: Pass extra build flags to compiler for parsing..

Moved to whitelist.

Wed, Jul 10, 10:44 PM · KDevelop
aaronpuchert accepted D22197: Add working directory to clang parser..
Wed, Jul 10, 10:26 PM · KDevelop
bungeman updated the diff for D22197: Add working directory to clang parser..

Change to workingDirectory, add comment.

Wed, Jul 10, 8:21 PM · KDevelop
arrowd closed D22350: Remove invalid check from test_projectload test..
Wed, Jul 10, 7:15 AM · KDevelop

Tue, Jul 9

apol accepted D22350: Remove invalid check from test_projectload test..
Tue, Jul 9, 10:38 PM · KDevelop
arrowd requested review of D22350: Remove invalid check from test_projectload test..
Tue, Jul 9, 6:33 PM · KDevelop
apol abandoned D8158: KDevelop: decorate patch version string in development builds.
Tue, Jul 9, 5:45 PM · KDevelop
apol commandeered D8158: KDevelop: decorate patch version string in development builds.
Tue, Jul 9, 5:45 PM · KDevelop
apol abandoned D6184: restore horizontal scrollbar in the project manager plugin.
Tue, Jul 9, 5:44 PM · KDevelop
apol commandeered D6184: restore horizontal scrollbar in the project manager plugin.
Tue, Jul 9, 5:44 PM · KDevelop
kossebau closed D22160: Document tree view close on middle button.
Tue, Jul 9, 8:56 AM · KDevelop

Mon, Jul 8

aaronpuchert added a comment to D22182: Remove anchorClicked lock which causes deadlock.

The workaround being that we eliminate the try-lock behaviour in our locker constructors and move that timeout behaviour elsewhere?

Exactly.

Mon, Jul 8, 11:51 PM · KDevelop
tristanp updated the diff for D22160: Document tree view close on middle button.
  • Fix action boolean.
Mon, Jul 8, 5:14 PM · KDevelop
anthonyfieroni added inline comments to D22160: Document tree view close on middle button.
Mon, Jul 8, 5:07 PM · KDevelop
sredman added a comment to D17908: kdevelop-msvc.bat finds VS-2017 based on a registry key on Windows..

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

Mon, Jul 8, 4:13 PM · KDevelop
sredman added a comment to D21589: Duplicate Visual Studio 2017 hack for VS 2019.

Actually, there is a bit of a problem... The version of the file currently on master is bugged :(

Mon, Jul 8, 3:56 PM · KDevelop
sredman added a comment to D17908: kdevelop-msvc.bat finds VS-2017 based on a registry key on Windows..

Unfortunately, I have a problem with using the batch file from this diff on my system.

Mon, Jul 8, 3:42 PM · KDevelop
tristanp added a comment to D22160: Document tree view close on middle button.

I'm okay for the name, thanks a lot for the review and futur commit

Mon, Jul 8, 2:58 PM · KDevelop
sredman added a comment to D21589: Duplicate Visual Studio 2017 hack for VS 2019.

Ping? Please keep hammering the iron while hot :)

Mon, Jul 8, 1:20 PM · KDevelop
kossebau added a comment to D16218: [KDevelop/Core]: safe signal-handler implementation.

Sh@@t, sorry, I allowed some unrelated (and potential future) changes to pollute this version. Will fix tomorrow.

Mon, Jul 8, 1:17 PM · KDevelop
kossebau added a comment to D21589: Duplicate Visual Studio 2017 hack for VS 2019.

Ping? Please keep hammering the iron while hot :)

Mon, Jul 8, 1:14 PM · KDevelop
kossebau added inline comments to D22158: Navigation context uses theme color..
Mon, Jul 8, 1:13 PM · KDevelop
kossebau added a comment to D8158: KDevelop: decorate patch version string in development builds.

@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).

Mon, Jul 8, 1:06 PM · KDevelop
kossebau added a comment to D6184: restore horizontal scrollbar in the project manager plugin.

@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.

Mon, Jul 8, 12:54 PM · KDevelop
kossebau added a comment to D22160: Document tree view close on middle button.

@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"?

Mon, Jul 8, 12:41 PM · KDevelop
mswan added a comment to D22182: Remove anchorClicked lock which causes deadlock.

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.

Mon, Jul 8, 6:22 AM · KDevelop

Sun, Jul 7

aaronpuchert added a comment to D22182: Remove anchorClicked lock which causes deadlock.

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. ;)

Sun, Jul 7, 11:42 PM · KDevelop
mswan added a comment to D22182: Remove anchorClicked lock which causes deadlock.

If we make the lock explicit, we can just annotate the constructor with ACQUIRE[_SHARED](duChainLock) (using the macros from the docs).

Sun, Jul 7, 7:27 AM · KDevelop

Fri, Jul 5

kossebau added a comment to D22158: Navigation context uses theme color..

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.

Fri, Jul 5, 6:52 PM · KDevelop
kossebau added a comment to D22158: Navigation context uses theme color..

Interesting approach, might work out, thanks for doing this :)

Fri, Jul 5, 4:07 PM · KDevelop

Thu, Jul 4

aaronpuchert added a comment to D22182: Remove anchorClicked lock which causes deadlock.

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.

Thu, Jul 4, 6:52 PM · KDevelop
mswan added a comment to D22182: Remove anchorClicked lock which causes deadlock.

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.

Thu, Jul 4, 2:45 AM · KDevelop
mswan added a comment to D22182: Remove anchorClicked lock which causes deadlock.

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 an local constructor invocation.

Thu, Jul 4, 2:03 AM · KDevelop
aaronpuchert added a comment to D22182: Remove anchorClicked lock which causes deadlock.

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.

Thu, Jul 4, 1:42 AM · KDevelop
mswan added a comment to D22182: Remove anchorClicked lock which causes deadlock.

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.

Thu, Jul 4, 1:36 AM · KDevelop
mswan added a comment to D22182: Remove anchorClicked lock which causes deadlock.

Lock and unlock shouldn't be idempotent.

Thu, Jul 4, 1:32 AM · KDevelop
aaronpuchert added a comment to D22182: Remove anchorClicked lock which causes deadlock.

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?

Thu, Jul 4, 1:25 AM · KDevelop
mswan added a comment to D22182: Remove anchorClicked lock which causes deadlock.

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.

Thu, Jul 4, 1:18 AM · KDevelop
aaronpuchert added a comment to D22182: Remove anchorClicked lock which causes deadlock.

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.

Thu, Jul 4, 12:48 AM · KDevelop

Wed, Jul 3

aaronpuchert added inline comments to D22197: Add working directory to clang parser..
Wed, Jul 3, 11:18 PM · KDevelop
bungeman added inline comments to D22197: Add working directory to clang parser..
Wed, Jul 3, 3:06 PM · KDevelop
mswan added a comment to D22182: Remove anchorClicked lock which causes deadlock.

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.

Wed, Jul 3, 1:26 AM · KDevelop
aaronpuchert added inline comments to D22197: Add working directory to clang parser..
Wed, Jul 3, 12:29 AM · KDevelop
aaronpuchert added a comment to D22182: Remove anchorClicked lock which causes deadlock.

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.

Wed, Jul 3, 12:12 AM · KDevelop
aaronpuchert added inline comments to D22217: Pass extra build flags to compiler for parsing..
Wed, Jul 3, 12:04 AM · KDevelop

Tue, Jul 2

bungeman updated the diff for D22217: Pass extra build flags to compiler for parsing..

Work with gcc, still have a few failing tests to investigate.

Tue, Jul 2, 10:54 PM · KDevelop
bungeman updated the diff for D22197: Add working directory to clang parser..

Upload from arc to get everything filled out.

Tue, Jul 2, 6:10 PM · KDevelop
bungeman added a reviewer for D22217: Pass extra build flags to compiler for parsing.: KDevelop.
Tue, Jul 2, 6:02 PM · KDevelop
bungeman requested review of D22217: Pass extra build flags to compiler for parsing..
Tue, Jul 2, 6:00 PM · KDevelop
mswan added a reviewer for D22182: Remove anchorClicked lock which causes deadlock: KDevelop.
Tue, Jul 2, 2:42 AM · KDevelop

Mon, Jul 1

bungeman requested review of D22197: Add working directory to clang parser..
Mon, Jul 1, 7:05 PM · KDevelop
tristanp updated the diff for D22158: Navigation context uses theme color..
  • Rename m_xxxHighlight to xxxHiglight as they are public.
Mon, Jul 1, 8:36 AM · KDevelop
mswan updated the diff for D22182: Remove anchorClicked lock which causes deadlock.

Add now necessary locks to avoid recurrence of bug 386901.

Mon, Jul 1, 2:08 AM · KDevelop
mswan requested review of D22182: Remove anchorClicked lock which causes deadlock.
Mon, Jul 1, 1:55 AM · KDevelop

Sun, Jun 30

apol accepted D22160: Document tree view close on middle button.
Sun, Jun 30, 8:28 AM · KDevelop
apol added a comment to D22158: Navigation context uses theme color..

Patch looks good to me. Would you be able to offer screenshots using Breeze?

Sun, Jun 30, 8:26 AM · KDevelop
apol added a comment to D20548: Fix document switcher plugin with multiple splitted view..

Please clean the patch of whitespace changes.

Sun, Jun 30, 8:22 AM · KDevelop
tristanp added a reviewer for D22160: Document tree view close on middle button: KDevelop.
Sun, Jun 30, 7:34 AM · KDevelop
tristanp requested review of D22160: Document tree view close on middle button.
Sun, Jun 30, 7:33 AM · KDevelop

Sat, Jun 29

tristanp added a reviewer for D22158: Navigation context uses theme color.: KDevelop.
Sat, Jun 29, 8:47 PM · KDevelop
tristanp requested review of D22158: Navigation context uses theme color..
Sat, Jun 29, 8:46 PM · KDevelop