On Thu, Sep 13, 2012 at 5:57 AM, Pavel Sanda <sa...@lyx.org> wrote:
> Scott Kostyshak wrote:
>> Is the patch (attached in the first email) OK to go in?
>
> As said I don't think there is any performance profit in those cases
> so I would stick to the default style we use.
> IIRC end iterators are generally const so such changes looks fine
> for others like exceptions please grep sources to find
> the defaults :)
I have no problem with the patch not going in and do not want you to
spend more or your time on this (although I don't mind spending more
of my time on this; I like these type of conversations). Please feel
free to skip the rest of this email and reject the patch and I will
happily move on to other things.
I agree with you that there is no practical gain. My motivation was
consistency and readability. I do think the patch makes the code more
consistent.
I think you agreed that const_iterator for end is the most common. The
following confirms:
$ git grep "const_iterator end" | wc -l
181
$ git grep "const_iterator const end" | wc -l
48
$ git grep ":iterator const end" | wc -l
14
$ git grep ":iterator end" | wc -l
69
Regarding const catches, I do not think const is inconsistent. It
appears half-half:
$ git grep "catch (" | grep -v "(...)" | grep const
Buffer.cpp: catch (exception const & e) {
BufferList.cpp: } catch (ExceptionMessage const & message) {
LyX.cpp: } catch (ExceptionMessage const & message) {
LyX.cpp: } catch (ExceptionMessage const & message) {
factory.cpp: } catch (ExceptionMessage const & message) {
frontends/qt4/GuiApplication.cpp: catch (ExceptionMessage const & e) {
frontends/qt4/GuiApplication.cpp: catch (exception const & e) {
frontends/qt4/GuiSpellchecker.cpp: } catch (ExceptionMessage
const & message) {
frontends/qt4/GuiView.cpp: } catch (ExceptionMessage const & e) {
frontends/qt4/GuiView.cpp: catch (ExceptionMessage const & ex) {
insets/InsetBibtex.cpp: } catch (ExceptionMessage const & message) {
mathed/InsetMathHull.cpp: } catch (MathExportException const &) {}
mathed/InsetMathHull.cpp: } catch (MathExportException const &) {}
tex2lyx/tex2lyx.cpp: } catch (ExceptionMessage const & message) {
tex2lyx/tex2lyx.cpp: } catch (ExceptionMessage const & message) {
$git grep "catch (" | grep -v "(...)" | grep -v const
Buffer.cpp: catch (iconv_codecvt_facet_exception & e) {
Buffer.cpp: catch (EncodingException & e) {
Buffer.cpp: catch (iconv_codecvt_facet_exception & e) {
Encoding.cpp: } catch (EncodingException & /* e */) {
Paragraph.cpp: } catch (EncodingException & e) {
Paragraph.cpp: } catch (EncodingException & e) {
frontends/qt4/GuiCitation.cpp: } catch (lyx::regex_error & e) {
frontends/qt4/GuiCitation.cpp: catch (lyx::regex_error & e) {
frontends/tests/biblio.cpp: catch (boost::regex_error & regerr) {
graphics/PreviewLoader.cpp: catch (iconv_codecvt_facet_exception & e) {
insets/InsetListings.cpp: } catch
(EncodingException & /* e */) {
mathed/InsetMathString.cpp: } catch (EncodingException & e) {
support/linkback/LinkBackProxy.m: @catch (NSException * exception) {
support/linkback/LinkBackProxy.m: @catch (NSException * exception) {
As to why I suggest that non-const catches should be changed to const
catches and not the other way around (both would lead to more
consistency in this case), I think that const catches, where
appropriate, are more readable. The non-const catches in Paragraph.cpp
are because the catch modifies e and then rethrows it. Thus it makes
sense for them to be non-const. However, suppose you see a non-const
catch that has a lot of code in the body and then at the end you see
"throw(e)". Does the body modify e before it throws it? It would be
nice to say "probably, because it's non-const". You could just do a
"find" but "e" is so common. Even if "e" were more unique and "find"
found the correct instances, you would still have to differentiate
between the modifying commands and the non-modifying commands.
I do not think that the other changes in the patch are contrary to the
current style. I made them with the same thought as what Richard has:
On Mon, Sep 3, 2012 at 10:30 AM, Richard Heck <rgh...@lyx.org> wrote:
> I generally prefer to see const wherever it is possible, if only because it
> functions like documentation. Whoever wrote the code is telling me this
> variable will not change its value, etc.
However, I'd be happy to discuss individual ones or if you'd like me
to go through them one by one that's fine too.
Thanks a lot, Pavel. I know that you have better things to do than
talk with a newbie about style :)
Scott
