|
Notes |
|
|
(0022798)
|
|
Sean McBride
|
|
2010-11-02 13:30
|
|
|
I forgot to say that I favour changing things to use "" over <>. Also, a regex could make this fairly easy to do. In fact, I had it ready to commit to CVS, but then the git switch arrived... |
|
|
|
(0023096)
|
|
Hans Johnson
|
|
2010-11-09 08:28
|
|
I vote we stick with Hans' original changes. They follow the gnu
recommendations. The Microsoft recommendation seems strange and my
original interpretation was flawed.
Sorry for the noise.
Hans, BTW there are two topics opened with the same content
http://review.source.kitware.com/#change,330 [^]
and
http://review.source.kitware.com/#change,329 [^]
On Mon, Nov 8, 2010 at 11:47 PM, Bill Lorensen <bill.lorensen@gmail.com> wrote:
> Here is what gnu says:
> http://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html [^]
> #include <file>
> This variant is used for system header files. It searches for a
> file named file in a standard list of system directories. You can
> prepend directories to this list with the -I option (see Invocation).
> #include "file"
> This variant is used for header files of your own program. It
> searches for a file named file first in the directory containing the
> current file, then in the quote directories and then the same
> directories used for <file>. You can prepend directories to the list
> of quote directories with the -iquote option.
>
> So, at least according to gcc, Hans' changes are correct.
>
> Here is another rule I found at:
> http://www.learncpp.com/cpp-tutorial/19-header-files/ [^]
> Rule: Use angled brackets to include header files that come with the
> compiler. Use double quotes to include any other header files.
>
> Bill
>
> On Mon, Nov 8, 2010 at 6:21 PM, Bill Lorensen <bill.lorensen@gmail.com> wrote:
>> Sounds reasonable, but so does my logic and so does Microsofts'. And
>> they are each different.
>>
>> Consistency is the winner. We need to pick a consistent approach.
>> Right now, ITK is not consistent.
>>
>> I think we should investigate further. There are so many files
>> affected, we only want to do this once.
>>
>> Bill
>>
>> On Mon, Nov 8, 2010 at 5:52 PM, Sean McBride <sean@rogue-research.com> wrote:
>>> Bill,
>>>
>>> I would use "#include <>" only for files that are 'system level', that
>>> is: provided by the OS or C/C++ standard library. So:
>>>
>>> #include <stdlib.h>
>>> #include <iostream>
>>> #include <Cocoa/Cocoa.h>
>>>
>>> But I'd use "" for when ITK includes it's own files, even from examples
>>> and tests.
>>>
>>> Alas, I think there isn't any crystal-clear cross-platform guidance to
>>> be had... :( I just looked in K&R for example, and it's fully of words
>>> like 'system-dependent' :(
>>>
>>> Sean
>>>
>>>
>>>
>>> On Mon, 8 Nov 2010 17:30:01 -0500, Bill Lorensen said:
>>>
>>>>Sean,
>>>>
>>>>We are trying to resolve this bug. It is not clear to me what is the
>>>>proper use of #include. Microsoft has an opinion. Can you point us to
>>>>a C++ standard, cross platform strategy?
>>>>
>>>>Bill
>>>>
>>>>
>>>>On Mon, Nov 8, 2010 at 5:20 PM, Bill Lorensen <bill.lorensen@gmail.com>
>>> wrote:
>>>>> I think we should investigate further.
>>>>>
>>>>>
>>>>>
>>>>> On Mon, Nov 8, 2010 at 5:18 PM, Hans Johnson <hans-johnson@uiowa.edu>
>>> wrote:
>>>>>> Bill,
>>>>>>
>>>>>> I think that this is probably correct. That would imply that for all
>>>>header
>>>>>> files in ITK/Code/Common we should use '#include "itkImage.h"' and for all
>>>>>> header files in ITK/Code/Algorithms we should use '#include <itkImage.h>'
>>>>>>
>>>>>> Hans
>>>>>>
>>>>>> --
>>>>>> Hans J. Johnson, Ph.D.
>>>>>> Hans-johnson@uiowa.edu
>>>>>>
>>>>>> 278 GH
>>>>>> The University of Iowa
>>>>>> Iowa City, IA 52241
>>>>>> (319) 353 8587
>>>>>>
>>>>>>
>>>>>>> From: Bill Lorensen <bill.lorensen@gmail.com>
>>>>>>> Date: Mon, 8 Nov 2010 17:08:51 -0500
>>>>>>> To: Hans Johnson <hans-johnson@uiowa.edu>
>>>>>>> Cc: Luis Ibanez <luis.ibanez@kitware.com>
>>>>>>> Subject: Re: FW: Change in ITK[master]: 0011198: ITK code is inconsistent
>>>>>>> include usage
>>>>>>>
>>>>>>> Here is the Microsoft take on the issue:
>>>>>>> http://msdn.microsoft.com/en-us/library/36k2cdd4%28VS.71%29.aspx [^]
>>>>>>>
>>>>>>> I'm still looking for a more general c++ recommendation.
>>>>>>>
>>>>>>>
>>>>>>> On Mon, Nov 8, 2010 at 5:04 PM, Bill Lorensen
>>>><bill.lorensen@gmail.com> wrote:
>>>>>>>> Hans,
>>>>>>>>
>>>>>>>> In my mind, if an include is "internal" to a package/toolkit, use "",
>>>>>>>> if "external" use <>.
>>>>>>>>
>>>>>>>> I'll do some research on this,
>>>>>>>>
>>>>>>>> Aren't you at a meeting? Somewhere in Iowa?
>>>>>>>>
>>>>>>>> Bill
>>>>>>>>
>>>>>>>> On Mon, Nov 8, 2010 at 4:58 PM, Hans Johnson <hans-
>>>>johnson@uiowa.edu> wrote:
>>>>>>>>> Bill,
>>>>>>>>>
>>>>>>>>> Could you elaborate a little bit about why you think this?
>>>>>>>>>
>>>>>>>>> I think of "#include <>" as system level headers that are only
>>>>looked for in
>>>>>>>>> the system level.
>>>>>>>>>
>>>>>>>>> I suppose that from the perspective of the Examples and Testing,
>>>>the Core is
>>>>>>>>> a system level include.
>>>>>>>>>
>>>>>>>>> I can make either way work, but it needs to be consistent.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Hans
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> Hans J. Johnson, Ph.D.
>>>>>>>>> Hans-johnson@uiowa.edu
>>>>>>>>>
>>>>>>>>> 278 GH
>>>>>>>>> The University of Iowa
>>>>>>>>> Iowa City, IA 52241
>>>>>>>>> (319) 353 8587
>>>>>>>>>
>>>>>>>>> ------ Forwarded Message
>>>>>>>>> From: "Bill Lorensen (Code Review)" <gerrit2@public.kitware.com>
>>>>>>>>> Reply-To: Bill Lorensen <bill.lorensen@gmail.com>
>>>>>>>>> Date: Mon, 8 Nov 2010 16:52:27 -0500
>>>>>>>>> To: Hans Johnson <hans-johnson@uiowa.edu>
>>>>>>>>> Cc: Luis Ibanez <luis.ibanez@kitware.com>, Kent Williams
>>>>>>>>> <norman-k-williams@uiowa.edu>
>>>>>>>>> Subject: Change in ITK[master]: 0011198: ITK code is inconsistent
>>>>include
>>>>>>>>> usage
>>>>>>>>>
>>>>>>>>> Bill Lorensen has posted comments on this change.
>>>>>>>>>
>>>>>>>>> Change subject: 0011198: ITK code is inconsistent include usage
>>>>>>>>> ......................................................................
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Patch Set 1:
>>>>>>>>>
>>>>>>>>> Shouldn't the tests and examples use #include <>?
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> To view, visit http://review.source.kitware.com/329 [^]
>>>>>>>>> To unsubscribe, visit http://review.source.kitware.com/settings [^]
>>>>>>>>>
>>>>>>>>> Gerrit-MessageType: comment
>>>>>>>>> Gerrit-Change-Id: Ibeecd9e748b1154a9fb5fdc9f3295d81fc81ba40
>>>>>>>>> Gerrit-PatchSet: 1
>>>>>>>>> Gerrit-Project: ITK
>>>>>>>>> Gerrit-Branch: master
>>>>>>>>> Gerrit-Owner: Hans J. Johnson <hans-johnson@uiowa.edu>
>>>>>>>>> Gerrit-Reviewer: Bill Lorensen <bill.lorensen@gmail.com>
>>>>>>>>> Gerrit-Reviewer: Luis Ibanez <luis.ibanez@kitware.com>
>>>>>>>>> Gerrit-Reviewer: kent williams <norman-k-williams@uiowa.edu>
>>>>>>>>>
>>>>>>>>> ------ End of Forwarded Message
>>>
>>>
>>>
>>
> |
|
|
|
(0023542)
|
|
Hans Johnson
|
|
2010-11-26 12:05
|
|
|
These have been converted to use "" for all non-system includes. |
|
|
|
(0023572)
|
|
Sean McBride
|
|
2010-11-29 12:36
|
|
Much better, thanks! Though I believe there are a few remaining:
1) search for "#include <itksys" and you'll find a few...
2) search for "#include <@KWSYS_NAMESPACE@/" and you'll find a few more. |
|
|
|
(0023583)
|
|
Brad King
|
|
2010-11-30 13:06
|
|
The C99 standard specifies that the searches for <> includes and "" includes are implementation-defined. However, if the "" search fails then the preprocessor is required to pretend that it was <> and search again. AFAIK all compilers search "" in the same directory as the file where the #include appears, but some add other locations (e.g. MS looks next to the grandparent includer). Most compilers define the <> search path by -I options followed by some default places.
In summary:
(1) The <> search path is more consistent across compilers
(2) Use "" to look next to the current file regardless of -I options
(3) Order doesn't matter if all names are unique
As far as the @KWSYS_NAMESPACE@ appearances:
KWSys is shared by many projects and from ITK's point of view is a third-party library. Its convention is to use <> for includes. That will not change. |
|
|
|
(0023584)
|
|
Brad King
|
|
2010-11-30 13:32
|
|
> this requires me to configure my Xcode project such that both
> user and system include paths point to ITK
I'm not sure what this means, but perhaps it is talking about setting both HEADER_SEARCH_PATHS (translates to -I) and USER_HEADER_SEARCH_PATHS (translates to -iquote) in Xcode projects. The latter is certainly not necessary. The former will be used for both "" and <> includes. |
|
|
|
(0023585)
|
|
Hans Johnson
|
|
2010-11-30 13:43
|
|
|
I made all of ITK consistent. The KWSYS is beyond the control of the ITKv4 team, and they have good reasons for not changing that. |
|
|
|
(0023586)
|
|
Sean McBride
|
|
2010-11-30 14:00
|
|
Boy, you guys don't give much time for one to reply before closing! :) Are you aware that Mantis does not even allow one to add comments when a bug in closed? Also, isn't it traditional for a bug submitter to be given a chance to verify a fix before closing? Twice this has been closed without giving me a chance to verify/comment. I'm not trying to be unappreciative here, just offering constructive criticism of the procedure... Instead, Mantis's "resolved" state could be used when the developer has claimed something fixed, then moved to "closed" when either the submitter agrees or the managers overrule.
Anyway...
Brad, yes that is what was part of what I was referring to. I was hoping to use only USER_HEADER_SEARCH_PATHS and not HEADER_SEARCH_PATHS for ITK includes. The other reason for the bug was the stylistic inconsistency. I should have time this week to try this again in Xcode after Hans' changes.
Hans, I don't believe you have quite made all of ITK consistent, which is why I reopened previously. As I said in comment 23572, search for "#include <itksys". itkDynamicLoader.h does:
#include <itksys/DynamicLoader.hxx>
from the file name (itkDynamicLoader.h), I assume it's part of ITK, no? Most all other files use "" for itksys includes. |
|
|
|
(0023587)
|
|
Hans Johnson
|
|
2010-11-30 14:08
|
|
The last fix to itksys/Dynamic loader (and a few others) were included in a gerrit patch that should be in ITK master by the end of the day.
http://review.source.kitware.com/#change,468 [^]
====
Regarding the closing of tickets, we are being aggressive about closing tickets. We realize that this resolved state is intended for the purpose that you describe, but in practice very few people observe that. It has been my personal experience that closing the ticket (while being more annoying) has the side benefit of people paying attention to it. To be honest, I've closed over 100 tickets this way in the past two months, and only 2-3 have been re-opened.
It saves the developer a lot of time managing tickets, and there is a better chance that the bug tracker can get to a manageable number of tickets. |
|
|
|
(0023640)
|
|
Hans Johnson
|
|
2010-12-01 09:41
|
|
|
Changes merged into ITK master. |
|