On Thu, Dec 10, 2015 at 05:52:15PM -0500, Richard Heck wrote:
> On 12/10/2015 03:09 AM, Scott Kostyshak wrote:
> > On Wed, Dec 09, 2015 at 10:10:42AM +0100, Jean-Marc Lasgouttes wrote:
> >> Le 09/12/2015 06:54, Scott Kostyshak a écrit :
> >>> Regarding the following code:
> >>>
> >>> -----
> >>> void Text::selectWord(Cursor & cur, word_location loc)
> >>> {
> >>> LBUFERR(this == cur.text());
> >>> CursorSlice from = cur.top();
> >>> CursorSlice to = cur.top();
> >>> getWord(from, to, loc);
> >>> -----
> >>>
> >>> It is not easy to know whether "to" and "from" are equal to each other
> >>> because
> >>> cur.top() might return something different the second time. For
> >>> readability
> >>> purposes, I would prefer either
> >> I do not really see why this confuses you actually.
> > Well, since I have a bad memory I always try to write code imagining
> > "what if I did not have any inside information". A newcommer does not
> > know that the initializations of 'to' and 'from' are useless (neither
> > would he under my proposal).
>
> And it's even more confusing because the initiation of from DOES matter,
> whereas that of to does not matter.
Good point. I did not think about this. I was more focused on the
readability before getWord ('from' and 'to' are equal but we only know
that if we know that cur.top() does not change anything). But you
brought up a deeper issue, which is that getWord() does not care what
'to' is initialized as.
> > A newcommer also does not know what top()
> > does. Maybe it pops off something, and thus changes the underlying list
> > so that the second call to it returns something different. My proposal
> > at least clarifies this.
> >
> > I don't care that much. I was just curious what others thought. This was
> > a poor example. I should have cut off the "getWord" call in my quote so
> > as just to focus on what the reader would think of 'from' and 'to'.
>
> At the very least, if the value of to is irrelevant, then it should just be:
> CursorSlice to;
> Then it's not potentially misleading as to why we're passing the same
> value twice.
I agree. I thought of another advantage. If there a bug and we somehow
use 'to' before it is overwritten, then it will go unnoticed. With the
attached patch, there should be a compiler warning about reading a
variable before writing to it. Is that reasoning correct?
Attached patch OK? If so, I would put it in at the beginning of the
2.3.0 cycle.
Scott
From 0edbc7f52f4ecb288389e94f87e7388d5c466166 Mon Sep 17 00:00:00 2001
From: Scott Kostyshak <skost...@lyx.org>
Date: Fri, 18 Dec 2015 21:58:22 -0500
Subject: [PATCH] Do not initialize a var to a val that's never used
By initializing 'to' to a value, the code made it seem like that
value mattered. But the value is overwritten in getWord().
Further, now if 'to' is used before it is initialized, there might
be a useful compiler warning that could point to a bug.
---
src/Text.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/Text.cpp b/src/Text.cpp
index 80babde..fe59cb3 100644
--- a/src/Text.cpp
+++ b/src/Text.cpp
@@ -1266,7 +1266,7 @@ void Text::selectWord(Cursor & cur, word_location loc)
{
LBUFERR(this == cur.text());
CursorSlice from = cur.top();
- CursorSlice to = cur.top();
+ CursorSlice to;
getWord(from, to, loc);
if (cur.top() != from)
setCursor(cur, from.pit(), from.pos());
--
2.1.4
signature.asc
Description: PGP signature
