Re: Review Request 127813: Process paths just once

[Matthew Dawson]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127813/#review95399
---



General +1 from me, just two things:

1) Can you split these two change into two commits?  One being the part about 
not removing file: twice, the second about removing duplicate homes?  If not, 
np.
2) Do you have any benchmark numbers about this?  Just looking at comparisions, 
it will take three comparisions each time to check the list, then at least one 
more comparision assuming they are all the same.  Versus only three 
comparisions on the old code.  And if any path does clean the string, the other 
comparisions are skipped in the old version.  That being said, if benchmarks 
suggest your version is faster in wall time (not callgrind counts), I have no 
problem taking it.

- Matthew Dawson


On May 4, 2016, 9:02 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127813/
> ---
> 
> (Updated May 4, 2016, 9:02 p.m.)
> 
> 
> Review request for KDE Frameworks and Matthew Dawson.
> 
> 
> Repository: kconfig
> 
> 
> Description
> ---
> 
> Just process every home path once, as the 3 alternatives will probably just 
> be the same.
> Don't drop the file: prefix twice in every run.
> 
> 
> Diffs
> -
> 
>   src/core/kconfiggroup.cpp 39d2441 
> 
> Diff: https://git.reviewboard.kde.org/r/127813/diff/
> 
> 
> Testing
> ---
> 
> Tests pass, apps work.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127813: Process paths just once

[Aleix Pol Gonzalez]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127813/#review95393
---



ping?

- Aleix Pol Gonzalez


On May 5, 2016, 3:02 a.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127813/
> ---
> 
> (Updated May 5, 2016, 3:02 a.m.)
> 
> 
> Review request for KDE Frameworks and Matthew Dawson.
> 
> 
> Repository: kconfig
> 
> 
> Description
> ---
> 
> Just process every home path once, as the 3 alternatives will probably just 
> be the same.
> Don't drop the file: prefix twice in every run.
> 
> 
> Diffs
> -
> 
>   src/core/kconfiggroup.cpp 39d2441 
> 
> Diff: https://git.reviewboard.kde.org/r/127813/diff/
> 
> 
> Testing
> ---
> 
> Tests pass, apps work.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127813: Process paths just once

[Aleix Pol Gonzalez]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127813/
---

(Updated May 5, 2016, 3:02 a.m.)


Review request for KDE Frameworks and Matthew Dawson.


Changes
---

Dropped static, converted back to QStringList.


Repository: kconfig


Description
---

Just process every home path once, as the 3 alternatives will probably just be 
the same.
Don't drop the file: prefix twice in every run.


Diffs (updated)
-

  src/core/kconfiggroup.cpp 39d2441 

Diff: https://git.reviewboard.kde.org/r/127813/diff/


Testing
---

Tests pass, apps work.


Thanks,

Aleix Pol Gonzalez

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127813: Process paths just once

[Andreas Hartmetz]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127813/#review95192
---




src/core/kconfiggroup.cpp (line 439)


Talking about same behavior, this also randomizes the order in case there 
are actually several home paths... however that happens.


- Andreas Hartmetz


On May 2, 2016, 4:32 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127813/
> ---
> 
> (Updated May 2, 2016, 4:32 p.m.)
> 
> 
> Review request for KDE Frameworks and Matthew Dawson.
> 
> 
> Repository: kconfig
> 
> 
> Description
> ---
> 
> Just process every home path once, as the 3 alternatives will probably just 
> be the same.
> Don't drop the file: prefix twice in every run.
> 
> 
> Diffs
> -
> 
>   src/core/kconfiggroup.cpp 39d2441 
> 
> Diff: https://git.reviewboard.kde.org/r/127813/diff/
> 
> 
> Testing
> ---
> 
> Tests pass, apps work.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127813: Process paths just once

[Albert Astals Cid]

> On May 2, 2016, 8:21 p.m., Albert Astals Cid wrote:
> > src/core/kconfiggroup.cpp, line 442
> > 
> >
> > Are we sure we want this to be static?
> 
> Aleix Pol Gonzalez wrote:
> I'm not sure, my impression is that it's better if we only fetch this 
> once, but I can easily be convinced otherwise.

My rationale is that the old code adapted to the new values if they changed, if 
it doesn't really give us much having it static (which i think it's not a huge 
win) preserving the same behaviour looks like a good thing.


- Albert


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127813/#review95112
---


On May 2, 2016, 4:32 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127813/
> ---
> 
> (Updated May 2, 2016, 4:32 p.m.)
> 
> 
> Review request for KDE Frameworks and Matthew Dawson.
> 
> 
> Repository: kconfig
> 
> 
> Description
> ---
> 
> Just process every home path once, as the 3 alternatives will probably just 
> be the same.
> Don't drop the file: prefix twice in every run.
> 
> 
> Diffs
> -
> 
>   src/core/kconfiggroup.cpp 39d2441 
> 
> Diff: https://git.reviewboard.kde.org/r/127813/diff/
> 
> 
> Testing
> ---
> 
> Tests pass, apps work.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127813: Process paths just once

[Aleix Pol Gonzalez]

> On May 2, 2016, 10:21 p.m., Albert Astals Cid wrote:
> > src/core/kconfiggroup.cpp, line 442
> > 
> >
> > Are we sure we want this to be static?

I'm not sure, my impression is that it's better if we only fetch this once, but 
I can easily be convinced otherwise.


- Aleix


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127813/#review95112
---


On May 2, 2016, 6:32 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127813/
> ---
> 
> (Updated May 2, 2016, 6:32 p.m.)
> 
> 
> Review request for KDE Frameworks and Matthew Dawson.
> 
> 
> Repository: kconfig
> 
> 
> Description
> ---
> 
> Just process every home path once, as the 3 alternatives will probably just 
> be the same.
> Don't drop the file: prefix twice in every run.
> 
> 
> Diffs
> -
> 
>   src/core/kconfiggroup.cpp 39d2441 
> 
> Diff: https://git.reviewboard.kde.org/r/127813/diff/
> 
> 
> Testing
> ---
> 
> Tests pass, apps work.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127813: Process paths just once

[Albert Astals Cid]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127813/#review95112
---




src/core/kconfiggroup.cpp (line 439)


Are we sure we want this to be static?


- Albert Astals Cid


On May 2, 2016, 4:32 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127813/
> ---
> 
> (Updated May 2, 2016, 4:32 p.m.)
> 
> 
> Review request for KDE Frameworks and Matthew Dawson.
> 
> 
> Repository: kconfig
> 
> 
> Description
> ---
> 
> Just process every home path once, as the 3 alternatives will probably just 
> be the same.
> Don't drop the file: prefix twice in every run.
> 
> 
> Diffs
> -
> 
>   src/core/kconfiggroup.cpp 39d2441 
> 
> Diff: https://git.reviewboard.kde.org/r/127813/diff/
> 
> 
> Testing
> ---
> 
> Tests pass, apps work.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 127813: Process paths just once

[Aleix Pol Gonzalez]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127813/
---

Review request for KDE Frameworks and Matthew Dawson.


Repository: kconfig


Description
---

Just process every home path once, as the 3 alternatives will probably just be 
the same.
Don't drop the file: prefix twice in every run.


Diffs
-

  src/core/kconfiggroup.cpp 39d2441 

Diff: https://git.reviewboard.kde.org/r/127813/diff/


Testing
---

Tests pass, apps work.


Thanks,

Aleix Pol Gonzalez

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel