Ticket #2446 (assigned defect)

Opened 6 years ago

Last modified 6 years ago

Rename Image, Mask, (Variance) typedefs in MaskedImage

Reported by: jbosch Owned by: rhl
Priority: normal Milestone:
Component: afw Keywords:
Cc: rhl, rowen, bick, cloomis, price, jbosch Blocked By:
Blocking: Project: LSST
Version Number:
How to repeat:

not applicable

Description

gcc 4.6 seems to (sometimes) think the Image and Mask typedefs in MaskedImage are illegal, because they change the meaning of those tokens from templates to types:

include/lsst/afw/image/MaskedImage.h:93:50: error: declaration of 'typedef class lsst::afw::image::Image<ImagePixelT> lsst::afw::image::MaskedImage<ImagePixelT, MaskPixelT, VariancePixelT>::Image' [-fpermissive]
include/lsst/afw/image/Image.h:408:11: error: changes meaning of 'Image' from 'class lsst::afw::image::Image<VariancePixelT>' [-fpermissive]

I say "sometimes" because everything compiles as it is now, but what should be innocent and unrelated changes trigger a compiler error. I noticed it when I tried changing

typedef typename Image<ImagePixelT> ImagePtr;

to

typedef PTR(Image<ImagePixelT>) ImagePtr;

This is a few lines above where the offending typedefs occur, but a comment next to those typedefs implies that only lines below them should be affected by the name shadowing.

I'm not sure what the C++ standard says about this, but I think those typedefs are dangerous even if this is actually a gcc bug, and I'd prefer renaming them to "ImageT" and "MaskT" (along with "Variance"->"VarianceT" for consistency).

That will require changes to a lot of downstream code, but as it is it's hard to change MaskedImage.h much without trigging a compilation failure on gcc.

Change History

comment:1 Changed 6 years ago by DefaultCC Plugin

  • Cc rhl, rowen, bick, cloomis, price, jbosch added

comment:2 Changed 6 years ago by rhl

JFB wrote:

I'd prefer renaming them to "ImageT" and "MaskT" (along with "Variance"->"VarianceT" for consistency).

I agree, but I think that that's forbidden by the coding standards (a rule that we ignore quite often). I think we should change the standard.

comment:3 Changed 6 years ago by jbosch

Ah. I agree on changing the standard. In the meantime, it's not quite as fragile as I thought, so it's not blocking anything I'm doing right now.

comment:4 Changed 6 years ago by robyn

The current C++ Standards Rule follows:

3-5. A name representing a typedef MUST be initial letter capitalized, camel-case with no prefix of the enclosing class and no suffix of 'T' or 'Type'.

typedef unsigned char Byte;
typedef unsigned long BitMask;
Byte smallMask;

This syntax is consistent with template type names and classes which are also similar in usage. The justification for the final clause is that all initial-capital camel-case names are types, so the 'T' or 'Type' suffix is redundant.

Note that formal arguments to templates are covered by 3-7.

Typedefs for specializations of templated types must still follow this rule. If the specialization uses a concrete type, the typedef name should typically include some indication of the parameter type (e.g. "typedef Image<float> ImageF;"). If the specialization uses an incoming template parameter, one option may be to use the specialized template's bare name, in cases where this will not create confusion (e.g. "typedef Image<PixelT> Image;").

comment:5 Changed 6 years ago by jbosch

To clarify the problem, using the language of the last paragraph of the standard text: using the specialized template's bare name invariably does cause confusion for compilers and (IMO) probably developers.

comment:6 Changed 6 years ago by robyn

From: 	Russell Owen <rowen@uw.edu>
Subject: 	Re: [LSST-data] Request for TCT Proposals
Date: 	Tue, 4 Dec 2012 13:06:22 -0800

For the record (and in case this can be resolved via email):

I agree with Jim. One situation where this comes up is wanting to use "any specialization of Image" or MaskedImage as a template parameter. In my opinion the clearest and name for the template parameter is ImageT (or MaskedImageT), but the existing standard forbids it. Using Image (or MaskedImage) makes the code harder to understand because the same name means both a templated class and a specialization of that class.

However, I personally prefer to continue to discourage a final T *unless* it prevents ambiguity.

Also, Jim did not address this, but I prefer T to Type and Jim's wording suggests that he agrees.

==============================================================================================

From: 	Kian-Tat Lim <ktl@slac.stanford.edu>
Subject: 	Re: [LSST-data] Request for TCT Proposals
Date: 	Tue, 4 Dec 2012 15:49:24 -0800

Russell wrote:

I agree with Jim. One situation where this comes up is wanting to use "any specialization of Image" or MaskedImage as a template parameter. In my opinion the clearest and name for the template parameter is ImageT (or MaskedImageT), but the existing standard forbids it. Using Image (or MaskedImage) makes the code harder to understand because the same name means both a templated class and a specialization of that class.

However, I personally prefer to continue to discourage a final T *unless* it prevents ambiguity.

Also, Jim did not address this, but I prefer T to Type and Jim's wording suggests that he agrees.

+1.

comment:7 Changed 6 years ago by robyn

  • Status changed from new to closed
  • Resolution set to fixed

The DM TCT agreed to the change. It has been updated in the C++ Standards Document

comment:8 Changed 6 years ago by robyn

  • Status changed from closed to assigned
  • Resolution fixed deleted

Oops, this was not only about getting the C++ Standards fixed. I'm reassigning it as open. Jim and Robert, you decide when this should be closed.

Note: See TracTickets for help on using tickets.