KScreenGenie moved to KDE Review

Boudhayan Gupta me at baloneygeek.com
Sat Jul 4 13:11:10 BST 2015


Hi Alexander,

>> No, some of them stay over in the header file. "Everything in one
>> place", for a small application such as this, which is not a public
>> library, takes precedence over minorly increased build times.
>
> Again, if you follow the links [1,2], then Foo.cpp should have
> #include <QString> even if Foo.h already contains it, in order to
> account for possible removal of that #include from Foo.h in the
> future. In this way, Foo.cpp acts like "one place for everything", the
> header #including only a minimal set files and minimal forward
> declarations to make it compile.

I read through that part. I just find it more comfortable to make
Foo.h the "one place for everything". I can guarantee that <QString>
will never be removed from Foo.h, so I don't see this being a problem.

>> That said, having to go through the commitdiffs for *four* unannounced
>> commits, two of which were major, to find the *one* location where the
>> real fix was and isolating that, is a huge waste of time which I could
>> have otherwise used to make important pending fixes to the UI and
>> program behaviour.
>
> Sorry, I didn't expect the #include change to be questionable. Will
> try to make my future commits more atomic.

I'm also sorry for going ballistic in the last mail :-) That was
probably uncalled for.

When I first pushed the project from GitHub into KDE, I asked on one
of the IRC channels whether the placement of includes is going to be a
problem, since all other KDE projects seem to be placing the includes
in the cpp files. I don't recall there being any negative comments,
which is why I didn't change it. This is how I've been doing it since
middle school (headers in the headers), and I find it natural this
way, like every header is a self-contained module, with the cpp file
being the supporting actual code.

I should also make clear that I didn't revert the commit because it
was unannounced or because of spite, but because I genuinely wanted
the cumulative headers.

I apologise again for the hot-headedness,

-- Boudhayan




More information about the kde-core-devel mailing list