>Martin Sebor wrote: > >I haven't gone through the whole patch yet so just a few brief >comments. I'll follow up with more once I've finished reviewing >the rest.
Thanks. I already have a bunch of changes for gcc 4.3, so it might be better to hold off. As you might expect MSVC allows some stuff that GCC doesn't. >First, some of your files are missing the final newline (required >of text files by POSIX). > >Regarding style: The convention is > > template <class> > struct S; > >rather than > > template <class> struct S; > Will be fixed for next review. >Regarding Copyrights: the year should be 2008 in all newly written >code, and there's a missing ", Inc." at the end. > Darn. I thought I had copied the block from an up-to-date file. I'll fix this also. >Regarding naming: > > * File names should be abbreviated so as not to exceed the 14 > character minimum required by POSIX. I wasn't aware of a 14 character limit, and I'm guessing that this is to do with the length of the header name as we don't limit the length of the test names or config test names in any way. If this is the case I may opt to merge the headers. i.e. _rw_has.h > * Do the trait member (e.g., _C_type, _C_value) need to be > privatized or would it make more sense to use the required names? I'm not sure what you're saying here. The types in the public interface all have the required names [from the base integral_constant template]. Everything else is an internal class, and I thought that we always needed to use privatized names for internal classes exposed in the headers. Using names consistent with those in the public interface doesn't buy us anything significant that I can think of, and there is no requirement that I know of saying we should. Other than avoiding the additional three characters, Is there any motivation to switch? The _C_ thing is starting to grow on me. > >Regarding the supporting infrastructure (config tests and headers): > > * In etc/config/src/RVALUE_REFERENCE.cpp needs, the _TYPENAME > macro isn't defined at this stage. Also, I'm not sure why > the test needs to use templates at all. You are right about the typename macro. I was being lazy and just copied a snippet of code out of the rvalue-reference proposal. > * In etc/config/src/VARIADIC_TEMPLATES.cpp, shouldn't struct > pack (the primary template) be defined? No. I'm still a little new to this, but what you are calling the primary template is just a fwd declaration. If you supply one or more template arguments, the first specialization will be used, otherwise the empty specialization will be used. > Also, is the empty specialization of pack valid? I'm pretty sure it is. 14.3 p4 says When template argument packs or default template- arguments are used, a template argument list can be empty. In that case the empty <> brackets shall still be used as the template argument list. The code is taken from Doug Gregor's Variadic Templates paper [http://tinyurl.com/48s8dr]. This paper is referenced from Proposed Wording for Variadic Templates [http://tinyurl.com/47n9uf] which appears to be the basis for the the wording in the current draft and the gcc-4 implementation. Perhaps I should write a simpler test. > Finally, even though it may > not matter for correctness, the pack specialization should > probably be defined as a struct rather than class. Agreed. > * In include/rw/_config-msvc.h: Would we have a naming conflict > with any of our other macros if we dropped the _TT_ bit from > the type traits macros? (I would prefer to keep the names of > the macros short and as close to the type traits intrinsics > as possible). Well, I was supplying macros for the internal type trait names, but they don't really do anything other than cut down on the number of characters to type. If I eliminated, or renamed those, then I would be able to use the more appropriate names. > * In include/rw/_defs.h, what is __NATIVE_TYPE_TRAITS_SUPPORTED > and where does it come from? It is from Howard Hinnant's original draft proposal for compiler support for type traits [http://tinyurl.com/3ez8ol]. I recently found James Widman's Compiler Support for type_traits proposal [http://tinyurl.com/3tyytp] and it removes the requirement for this macro. > I'm also concerned that the macro > might be #defined but not all the traits might be supported or > one of them might be spelled differently. Yeah, the original proposal said that the macro would be defined if the compiler provided 'this interface', so I think it would need to provide exactly those names. Since the macro was removed from the current document I don't believe that I can rely on it, so I'll be removing the block from rw/_defs.h and add platform specific blocks somewhere else [probably in the _config-<compiler>.h files for now]. > >I also noticed that some files (e.g., _is_floating_point.h) make >use of the _RWSTD_SPECIALIZED_CLASS macro while others assume >explicit specialization is supported. I suspect we might as well >assume it is and do away with the macro. Ditto for _TYPENAME. > I tried to use the macros everywhere. Since we haven't finished up the discussion about what compiler features we can expect, I figured I had better stick with what we're already doing. >Martin > I'll try to post the updated patch this afternoon. That might get pushed back to tomorrow if the merge from 4.3 to trunk takes any more than a few hours. Travis
