Shachar Shemesh wine-patches@shemesh.biz writes:
-#include "config.h" -#include "wine/port.h" +#include <config.h> +#include <wine/port.h>
#include <stdlib.h> #include <string.h> #include <assert.h> -#include "winerror.h" -#include "winnls.h" -#include "wownt32.h" -#include "gdi.h" -#include "wine/unicode.h" -#include "wine/debug.h" +#include <winerror.h> +#include <winnls.h> +#include <wownt32.h> +#include <gdi.h> +#include <wine/unicode.h> +#include <wine/debug.h> +#include "gdibidi.h"
Please don't change include quotes when modifying a file. It's not necessary, and only adds confusion.
Alexandre Julliard wrote:
Shachar Shemesh wine-patches@shemesh.biz writes:
-#include "config.h" -#include "wine/port.h" +#include <config.h> +#include <wine/port.h>
#include <stdlib.h> #include <string.h> #include <assert.h> -#include "winerror.h" -#include "winnls.h" -#include "wownt32.h" -#include "gdi.h" -#include "wine/unicode.h" -#include "wine/debug.h" +#include <winerror.h> +#include <winnls.h> +#include <wownt32.h> +#include <gdi.h> +#include <wine/unicode.h> +#include <wine/debug.h> +#include "gdibidi.h"
Please don't change include quotes when modifying a file. It's not necessary, and only adds confusion.
The reason I did was to reduce confusion. The usual includes are standard includes, and can be included with either <> or "". The new include (gdibidi.h) is a local include, and can only be included with "". To differentiate the two, I changed those who could afford a <>.
Shachar
Shachar Shemesh wine-devel@shemesh.biz writes:
The reason I did was to reduce confusion. The usual includes are standard includes, and can be included with either <> or "". The new include (gdibidi.h) is a local include, and can only be included with "". To differentiate the two, I changed those who could afford a <>.
Local includes can be included with <> too, there's no reason to make a distinction. And some of the headers in include/ are actually internal, like gdi.h (actually I would argue that bidi definitions should go there). The fact is that all our source files use "" for both internal and exported headers, and we are not going to change all of them. Changing only a few here and there creates a lot more confusion than it solves.
First I want to clarify something. Nothing in this email is meant to suggest that I think the bidi change should, in any way, depend on this issue. If you want, I can even amend my patch.
Alexandre Julliard wrote:
Shachar Shemesh wine-devel@shemesh.biz writes:
The reason I did was to reduce confusion. The usual includes are standard includes, and can be included with either <> or "". The new include (gdibidi.h) is a local include, and can only be included with "". To differentiate the two, I changed those who could afford a <>.
Local includes can be included with <> too,
Only because we add "-I." to the compilation flags. Adding "-I." to the compilation flags should not be necessary.
there's no reason to make a distinction.
Well, there is the minor point of "what do I mean by...". I don't think semantic hints are something to be discarded so easily.
And some of the headers in include/ are actually internal, like gdi.h (actually I would argue that bidi definitions should go there).
I placed them under the GDI directory, following what I thought was someone else's example. As chance would have it, I don't know what was that.
In any case, it makes much sense to me not to place non-exported headers in include. The idea is that you can tell packagers to take the entire "include" directory and ship it. Unless I misunderstand, and winelib apps actually NEED gdi.h, we do not wish to have it packaged. This leaves packagers with zen decisions - not a good thing.
The fact is that all our source files use "" for both internal and exported headers, and we are not going to change all of them.
Why not, really? I made the original change under the recollection that changing them all was, in fact, the future plan. As such, I though I'll mark this particular file as "already translated", so that they know not to change "gdibidi.h", and thus break compilation.
Changing only a few here and there creates a lot more confusion than it solves.
Alexandre, I only partially agree. I think the current situation, where "" and <> behave the same, is an undesireable one. We want to be able to tell packagers to grab the entire /include directory with no fear (libwine-devel.rpm anyone?). We don't want it to hold directories not available for the standard windows. In order to enforce this distinction, we need to remove the "-I." from the compilation options. The last stage, of changing "" to <> can then happen slowly.
Shachar
Shachar Shemesh wine-devel@shemesh.biz writes:
First I want to clarify something. Nothing in this email is meant to suggest that I think the bidi change should, in any way, depend on this issue. If you want, I can even amend my patch.
Yes please.
Only because we add "-I." to the compilation flags. Adding "-I." to the compilation flags should not be necessary.
It is necessary, this has been discussed before.
In any case, it makes much sense to me not to place non-exported headers in include. The idea is that you can tell packagers to take the entire "include" directory and ship it. Unless I misunderstand, and winelib apps actually NEED gdi.h, we do not wish to have it packaged. This leaves packagers with zen decisions - not a good thing.
gdi.h will be moved to dlls/gdi at some point. Which of course means that if we do things the way you suggest we then need to go back into all files and change <> back to "".
The fact is that all our source files use "" for both internal and exported headers, and we are not going to change all of them.
Why not, really?
Because it isn't broken. We need to fix exported headers to use <> since it can make a difference in Winelib apps that use them; but in Wine source files "" works just as well as <>.
Alexandre, I only partially agree. I think the current situation, where "" and <> behave the same, is an undesireable one. We want to be able to tell packagers to grab the entire /include directory with no fear (libwine-devel.rpm anyone?).
Yes, include/ should contain only exported headers, it's part of the dll separation work, we are getting there. It's completely orthogonal to whether we use <> or "" to include them.
Alexandre Julliard wrote:
Shachar Shemesh wine-devel@shemesh.biz writes:
First I want to clarify something. Nothing in this email is meant to suggest that I think the bidi change should, in any way, depend on this issue. If you want, I can even amend my patch.
Yes please.
Wilco.
Only because we add "-I." to the compilation flags. Adding "-I." to the compilation flags should not be necessary.
It is necessary, this has been discussed before.
I'll try to find the discussion (if someone has a handy pointer - it would be greatly apretiated), but if it turns out that it is necessary because we use "" instead of <>, I don't think that counts ;-)
In any case, it makes much sense to me not to place non-exported headers in include. The idea is that you can tell packagers to take the entire "include" directory and ship it. Unless I misunderstand, and winelib apps actually NEED gdi.h, we do not wish to have it packaged. This leaves packagers with zen decisions - not a good thing.
gdi.h will be moved to dlls/gdi at some point. Which of course means that if we do things the way you suggest we then need to go back into all files and change <> back to "".
No. It just means we shouldn't change it to <>, now or ever.
The fact is that all our source files use "" for both internal and exported headers, and we are not going to change all of them.
Why not, really?
Because it isn't broken. We need to fix exported headers to use <> since it can make a difference in Winelib apps that use them; but in Wine source files "" works just as well as <>.
Ok, how about if I send you a perl program that goes over the wine include folder, searches for each file found there in the MS SDK, and builds a list of exported headers. It will then go over the wine source, and change only those headers from "" to <>. This way, no manual work, no changing "" to <> and then back (as gdi.h will, obviously, not be in that list), and we have a clear consistant policy that makes sense. This also solves the winelib problem you mentioned.
Alexandre, I only partially agree. I think the current situation, where "" and <> behave the same, is an undesireable one. We want to be able to tell packagers to grab the entire /include directory with no fear (libwine-devel.rpm anyone?).
Yes, include/ should contain only exported headers, it's part of the dll separation work, we are getting there. It's completely orthogonal to whether we use <> or "" to include them.
I can see why you say that, but I feel it narrows the discussion down to technical (will or will not compile) consideration only. I think that we also need to show commitment to separating inner from exported, and this, to me, means the source too.
Shachar
Shachar Shemesh wine-devel@shemesh.biz writes:
I can see why you say that, but I feel it narrows the discussion down to technical (will or will not compile) consideration only. I think that we also need to show commitment to separating inner from exported, and this, to me, means the source too.
I don't think it's a useful distinction to make in the source; IMO the current situation is just as useful, since it lets you distinguish between system headers and Wine headers. In a Winelib app, it makes sense to use <>, since a #include <winbase.h> and a #include <stdio.h> mean the same thing, they both include system headers from /usr/include. In the Wine source it's very different, a #include <winbase.h> will *not* include the system header from /usr/include, it will include the local header from the current source tree. That's an important distinction too, and one that we would lose by changing all includes to <>.
Both options make sense, and they both convey (different) useful information, so you can't say one is better than the other. And it doesn't make sense to change all the source files if the end result is not a clear improvement, which it isn't in that case.
Le mer 25/06/2003 à 16:01, Shachar Shemesh a écrit :
Only because we add "-I." to the compilation flags. Adding "-I." to the compilation flags should not be necessary.
It is necessary, this has been discussed before.
I'll try to find the discussion (if someone has a handy pointer - it would be greatly apretiated), but if it turns out that it is necessary because we use "" instead of <>, I don't think that counts ;-)
IIRC, it was something about building Wine out of tree. The (many) -I. or similar in gcc args while building Wine are for that, too.
Ok, how about if I send you a perl program that goes over the wine include folder, searches for each file found there in the MS SDK, and builds a list of exported headers. It will then go over the wine source, and change only those headers from "" to <>. This way, no manual work, no changing "" to <> and then back (as gdi.h will, obviously, not be in that list), and we have a clear consistant policy that makes sense. This also solves the winelib problem you mentioned.
There's a part of the janitorial page which says that Wine's headers include too much stuff, and should be trimmed down to what Windows headers include. Not exactly the same thing, but probably a good use of Perl (meant as an advertisement for the task, not asking you to do it Shachar).
Vincent
I'll try to find the discussion (if someone has a handy pointer - it would be greatly apretiated), but if it turns out that it is necessary because we use "" instead of <>, I don't think that counts ;-)
I believe that the difference between "" and <> is that if you use "" the directory where the file that contains the #include is looked in before the directories specified with -I and the 'standard' places.
This can make it VERY difficult to build with a local (aka hacked) copy of an include file. Also, if there are multiple files with the same name it may not be obvious which one is actually included.
IMHO Using <> throughout is actually best - unless you actually really, really want the file from the directory where the including file was found.
David
I'm sorry, so maybe it is not necessary, but how does doing it the 'right way' add confusion?
-- Jeff Smith
--- Alexandre Julliard julliard@winehq.org wrote:
Shachar Shemesh wine-patches@shemesh.biz writes:
-#include "config.h" -#include "wine/port.h" +#include <config.h> +#include <wine/port.h>
#include <stdlib.h> #include <string.h> #include <assert.h> -#include "winerror.h" -#include "winnls.h" -#include "wownt32.h" -#include "gdi.h" -#include "wine/unicode.h" -#include "wine/debug.h" +#include <winerror.h> +#include <winnls.h> +#include <wownt32.h> +#include <gdi.h> +#include <wine/unicode.h> +#include <wine/debug.h> +#include "gdibidi.h"
Please don't change include quotes when modifying a file. It's not necessary, and only adds confusion.
-- Alexandre Julliard julliard@winehq.com
__________________________________ Do you Yahoo!? SBC Yahoo! DSL - Now only $29.95 per month! http://sbc.yahoo.com
Jeff Smith whydoubt@yahoo.com writes:
I'm sorry, so maybe it is not necessary, but how does doing it the 'right way' add confusion?
There is no 'right way', inside the source tree both are completely equivalent. Changing it adds confusion because you now have 1000 files doing it one way and 3 files doing it differently for no good reason.
Alexandre Julliard julliard-at-winehq.org |Wine Mailing Lists| wrote:
Jeff Smith whydoubt@yahoo.com writes:
I'm sorry, so maybe it is not necessary, but how does doing it the 'right way' add confusion?
There is no 'right way', inside the source tree both are completely equivalent. Changing it adds confusion because you now have 1000 files doing it one way and 3 files doing it differently for no good reason.
This thread has been confusing me. I was under the impression that when appropriate people should be changing "" includes to <>.
I was wondering why I had this impression, and then remembered this section on the wine janatorial page:
`Include statements should use <> instead of ""`
Is this now incorrect? Confused -Rob.
-
Alexandre Julliard -
David Laight -
Jeff Smith -
Robert North -
Shachar Shemesh -
Vincent Béron