On Fri, Dec 18, 2015 at 10:13:29PM -0500, Scott Kostyshak wrote:
> 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
>
Bump.
Scott
signature.asc
Description: PGP signature
