Use of STL iterators in GiNaC
Hi, I'm trying to compile ginac-1.5.1 with VC++ under Windows. I know "I should get myself a better compiler", but that being said, I've encountered runtime problems due to usage of STL iterators that might be wrong, essentially on std::vector. Problems I've noticed are: dereferencing end(), incrementing end(), decrementing begin()... I'm currently tracking down these issues by running the test suite in the source package, but this might not catch all of them. If you are interested, I can send a patch once I succeed to run the test suite completely. Bye, Michael.
Michael Goffioul wrote:
Hi,
Hi Michael,
I'm trying to compile ginac-1.5.1 with VC++ under Windows. I know "I should get myself a better compiler", but that being said, I've encountered runtime problems due to usage of STL iterators that might be wrong, essentially on std::vector.
Problems I've noticed are: dereferencing end(), incrementing end(), decrementing begin()...
Up to VS 2003 these operations did not cause any trouble and behaved just like gcc. Since 2005 those lead to pointers in hyperspace and cause segfaults. I am not sure what the C++ standard says about such operations.
I'm currently tracking down these issues by running the test suite in the source package, but this might not catch all of them. If you are interested, I can send a patch once I succeed to run the test suite completely.
Cool. I am not a contributor to the ginac project, but I would love to see your patch.
Bye, Michael.
Cheers, Michael
_______________________________________________ GiNaC-list mailing list GiNaC-list@ginac.de https://www.cebix.net/mailman/listinfo/ginac-list
Hi,
Up to VS 2003 these operations did not cause any trouble and behaved just like gcc. Since 2005 those lead to pointers in hyperspace and cause segfaults. I am not sure what the C++ standard says about such operations.
I believe the behavior is "undefined". In any case, attempts to dereference end(), increment end() and decrement begin() definitely look like bugs... Regards, James.
Hi, Michael Goffioul schrieb:
I'm currently tracking down these issues by running the test suite in the source package, but this might not catch all of them. If you are interested, I can send a patch once I succeed to run the test suite completely.
yes, that would be interesting! I'd like to see where this happens. I don't have access to a VC compiler to try it myself, though. I see no problem with dereferencing end(), and doing operations like end()+<integer>. In that case, end() should return a proper iterator as a temporary and on these the operations +,- are defined (somewhere 24.1.5 in the standard, I believe). But maybe there is a ++ or -- involved? Regards, Jens
On Thu, Apr 16, 2009 at 12:10 PM, Jens Vollinga <jensv@nikhef.nl> wrote:
Hi,
Michael Goffioul schrieb:
I'm currently tracking down these issues by running the test suite in the source package, but this might not catch all of them. If you are interested, I can send a patch once I succeed to run the test suite completely.
yes, that would be interesting! I'd like to see where this happens. I don't have access to a VC compiler to try it myself, though.
(ref your patch) There are more issues than the ones you mention, at least with VC++. I'll provide a full patch in the coming days, but here are some examples: ncmul.cpp: ncmul::eval(int), ~line 350 rettypes has the right capacity (due to the "reserve" call), but a size of 0. Then using "rettypes[0]" makes VC++ to raise an exception, due to internal bound checking. About this, I'm not sure what the standard says, but when I look at http://www.sgi.com/tech/stl/RandomAccessContainer.html the precondition is not met. utils.h: permutation_sign(), ~line 184 If I understand the code correctly, "other" is kept one position before "i". When "i" reaches "first", "other" is one position before. But when "first"equals std::vector::begin(), you end up decrementing begin(). factor.cpp: modular_matrix::mul_col(), ~line 697 When rc == (r-1), "i" gets past end().
I see no problem with dereferencing end(), and doing operations like end()+<integer>. In that case, end() should return a proper iterator as a temporary and on these the operations +,- are defined (somewhere 24.1.5 in the standard, I believe). But maybe there is a ++ or -- involved?
I'm no C++ standard expert, but when I look at http://www.sgi.com/tech/stl/RandomAccessIterator.html my understanding of the precondition for "i += n" is that you cannot go after end(). To be honest, I know VC++ is by far the worst standard compliant compiler, but in all the situations mentioned above, I had the impression that ginac's code relied on undefined behaviors that happen to work fine on gcc/glibc. Bye, Michael.
Hi, Michael Goffioul schrieb:
(ref your patch) There are more issues than the ones you mention, at least with VC++. I'll provide a full patch in the coming days, but here are some examples:
that would be very nice!
ncmul.cpp: ncmul::eval(int), ~line 350 rettypes has the right capacity (due to the "reserve" call), but a size of 0. Then using "rettypes[0]" makes VC++ to raise an exception, due to internal bound checking. About this, I'm not sure what the standard says, but when I look at http://www.sgi.com/tech/stl/RandomAccessContainer.html the precondition is not met.
Here I agree. The code is not clean. Either we should do a proper resize at the beginning or use some push_back() instead of [].
utils.h: permutation_sign(), ~line 184 If I understand the code correctly, "other" is kept one position before "i". When "i" reaches "first", "other" is one position before. But when "first"equals std::vector::begin(), you end up decrementing begin().
factor.cpp: modular_matrix::mul_col(), ~line 697 When rc == (r-1), "i" gets past end().
I am not sure about the code in utils.h, but the code in factor.cpp looks perfectly fine to me. See below.
I'm no C++ standard expert, but when I look at http://www.sgi.com/tech/stl/RandomAccessIterator.html my understanding of the precondition for "i += n" is that you cannot go after end().
It seems that the standard (I only have the draft. Hopefully there is no difference to the final version in the areas under consideration) and the SGI documentation deviate from each other. In SGI there are pre- and postconditions on +, -, +=, and -=. And these conditions are violated in the examples as you point out. But in the standard +, -, +=, -= have NO pre-/postconditions to them! See 24.1.5. The confusing part is, that -- and ++ DO have such conditions. But for the ++ operation, it is explicitly allowed to go past-the-end. See 24.1.4. So no problem for the code in factor.cpp. The code in utils.h seems to be against the standard, though, but I am no language lawyer and so I am maybe misinterpreting the conditions there. In case, this can be fixed by using reverse iterators. Given the history of the STL, it is understandable that SGI deviates from the standard. But that Microsoft had to copy that archetype ..., well, here we go again ...
To be honest, I know VC++ is by far the worst standard compliant compiler, but in all the situations mentioned above, I had the impression that ginac's code relied on undefined behaviors that happen to work fine on gcc/glibc.
Anyway, I think it is still good to "fix" the code if possible, so your patches will be appreciated. Regards, Jens
On Thu, Apr 16, 2009 at 2:04 PM, Jens Vollinga <jensv@nikhef.nl> wrote:
I'm no C++ standard expert, but when I look at http://www.sgi.com/tech/stl/RandomAccessIterator.html my understanding of the precondition for "i += n" is that you cannot go after end().
It seems that the standard (I only have the draft. Hopefully there is no difference to the final version in the areas under consideration) and the SGI documentation deviate from each other. In SGI there are pre- and postconditions on +, -, +=, and -=. And these conditions are violated in the examples as you point out. But in the standard +, -, +=, -= have NO pre-/postconditions to them! See 24.1.5. The confusing part is, that -- and ++ DO have such conditions. But for the ++ operation, it is explicitly allowed to go past-the-end. See 24.1.4. So no problem for the code in factor.cpp. The code in utils.h seems to be against the standard, though, but I am no language lawyer and so I am maybe misinterpreting the conditions there. In case, this can be fixed by using reverse iterators.
OK, I had a look in a draft C++ standard document I could find on the web, and when reading 24.1.3, the "pre" condition for ++r is that r must be dereferenceable. When r == end(), it is not dereferenceable, so you cannot do ++r. Don't you have the same interpretation? In 24.1.4, the pre condition for --r is that there exists s such that r == ++s. For ++s to be valid, s must be dereferenceable, so s cannot be before begin(). Moreover, the post condition is that r is dereferenceable (there seems to be a typo in the version I have, which says s is dereferenceable, but this doesn't make sense), so r cannot be before begin(). In 24.1.5, there's no pre condition on "r += n", but I think it can be deduced from the operational semantics and the interpretation above that you cannot go past end(). My general understanding is: 1) you cannot go before begin() 2) you can go after the last element, but only by an offset of 1, which corresponds to end() Ok, my understanding may be wrong, but this is how Microsoft implemented it... :) Michael.
Hi, Michael Goffioul schrieb:
factor.cpp. The code in utils.h seems to be against the standard, though, but I am no language lawyer and so I am maybe misinterpreting the conditions there. In case, this can be fixed by using reverse iterators.
OK, I had a look in a draft C++ standard document I could find on the web, and when reading 24.1.3, the "pre" condition for ++r is that r must be dereferenceable. When r == end(), it is not dereferenceable, so you cannot do ++r. Don't you have the same interpretation?
Ooops, after re-reading it: yes I do, now. I was too focused on the post-condition ...
In 24.1.4, the pre condition for --r is that there exists s such that r == ++s. For ++s to be valid, s must be dereferenceable, so s cannot be before begin(). Moreover, the post condition is that r is dereferenceable (there seems to be a typo in the version I have, which says s is dereferenceable, but this doesn't make sense), so r cannot be before begin().
I agree. Actually, I agreed -- halfheartedly ;-) -- already in my last mail (see citation above). But I am not sure about the reverse iterator fix anymore.
In 24.1.5, there's no pre condition on "r += n", but I think it can be deduced from the operational semantics and the interpretation above that you cannot go past end().
There you're right as well. Another thing I overlooked ...
My general understanding is: 1) you cannot go before begin() 2) you can go after the last element, but only by an offset of 1, which corresponds to end()
100% agreement, now. Thanks for the clarification!
Ok, my understanding may be wrong, but this is how Microsoft implemented it... :)
It seems they stick better to the rules than the gcc people ... *surprise* Regards, Jens
On Thu, Apr 16, 2009 at 3:02 PM, Jens Vollinga <jensv@nikhef.nl> wrote:
In 24.1.4, the pre condition for --r is that there exists s such that r == ++s. For ++s to be valid, s must be dereferenceable, so s cannot be before begin(). Moreover, the post condition is that r is dereferenceable (there seems to be a typo in the version I have, which says s is dereferenceable, but this doesn't make sense), so r cannot be before begin().
I agree. Actually, I agreed -- halfheartedly ;-) -- already in my last mail (see citation above). But I am not sure about the reverse iterator fix anymore.
The reverse iterator will be bound between its own begin() and end(), which translates into "last element" and "prior-the-start element". This may or may not help in fixing the code, but I don't have any clue about that.
My general understanding is: 1) you cannot go before begin() 2) you can go after the last element, but only by an offset of 1, which corresponds to end()
100% agreement, now. Thanks for the clarification!
No problem. I'm glad we agree now. Note that I'll produce a patch in the coming days with fixes for all the issues that popped up while trying to run the test suite, but there might actually be other issues that didn't show up. Michael.
On Thu, Apr 16, 2009 at 3:48 PM, Michael Goffioul <michael.goffioul@gmail.com> wrote:
No problem. I'm glad we agree now. Note that I'll produce a patch in the coming days with fixes for all the issues that popped up while trying to run the test suite, but there might actually be other issues that didn't show up.
Here's the promised patch. Note that it is not intended to be applied as-is to ginac source tree. But it's to show what are the required changes to make ginac compilable under VC++ and to be able to run the test suite. Some comments about the patch: - the __alignof__ vs. __alignof could be turned into a configure test - VC++ does not support __func__ macro, so I changed it into __FILE__ - in exprseq.cpp, I had to add a dummy function to make VC++ instantiate expreseq::info(); don't know why... - when trying to fix issues with iterators, I might have broken the normal flow of ginac (although the test suite seems to run fine), so these changes have to be reviewed - I'm pretty sure there are other locations with iterator issues, but they didn't produce any problem in the test suite, so I didn't notice them Bye, Michael.
Hi, On Thu, Apr 16, 2009 at 10:45:56PM +0100, Michael Goffioul wrote:
On Thu, Apr 16, 2009 at 3:48 PM, Michael Goffioul <michael.goffioul@gmail.com> wrote:
Here's the promised patch.
Thanks for a patch. Supporting more platforms/compilers is certainly a good thing as long as it doesn't break the code and doesn't introduce subtle changes. Unfortunately your patch *does* introduce subtle changes (and breaks compilation with any non-m$ compiler).
Note that it is not intended to be applied as-is to ginac source tree.
Anyway, it would be nice if you avoid introducing the breakage and semantic changes, or at least clearly mark them.
But it's to show what are the required changes to make ginac compilable under VC++ and to be able to run the test suite.
diff -ur ginac-1.5.1/ginac/ncmul.cpp ginac-1.5.1-new/ginac/ncmul.cpp --- ginac-1.5.1/ginac/ncmul.cpp 2009-02-17 13:39:22 +0000 +++ ginac-1.5.1-new/ginac/ncmul.cpp 2009-04-15 23:14:00 +0100 @@ -340,7 +340,7 @@ // determine return types unsignedvector rettypes; - rettypes.reserve(assocseq.size()); + rettypes.resize(assocseq.size()); I'm afraid this change breaks canonicalization of non-commutative products.
Some comments about the patch: - the __alignof__ vs. __alignof could be turned into a configure test
The patch as is breaks compilation with any non-m$ compiler. That's certainly not welcome.
- VC++ does not support __func__ macro, so I changed it into __FILE__
Stripping debugging info is not welcome either. Please #ifdef _MSVC (or whatever it is) these changes.
- in exprseq.cpp, I had to add a dummy function to make VC++ instantiate expreseq::info(); don't know why...
Please #ifdef _MSVC that function. Also it would be nice to have a comment in the source explaining why it's necessary (otherwise it's so tempting to delete that pointless code). Other changes, like diff -ur ginac-1.5.1/ginac/parser/parser.cpp ginac-1.5.1-new/ginac/parser/parser.cpp --- ginac-1.5.1/ginac/parser/parser.cpp 2009-02-17 13:39:22 +0000 +++ ginac-1.5.1-new/ginac/parser/parser.cpp 2009-04-15 20:59:47 +0100 @@ -82,7 +82,7 @@ } extern numeric* _num_1_p; -extern ex _ex0; +extern const ex _ex0; need such an #ifdef and a comment too. Best regards, Alexei P.S. (How) Did you manage to compile CLN with msvc?
Hi Alexei, Alexei Sheplyakov schrieb:
subtle changes. Unfortunately your patch *does* introduce subtle changes (and breaks compilation with any non-m$ compiler).
I think he is aware of that, since he announced the issue in his last email, and ...
Anyway, it would be nice if you avoid introducing the breakage and semantic changes, or at least clearly mark them.
this seems to be our job, not his. So, the question is: will you care about this or should I? We can discuss this on the devel-list if you like. Regards, Jens
Hi Jens, On Fri, Apr 17, 2009 at 11:20:57AM +0200, Jens Vollinga wrote:
I think he is aware of that, since he announced the issue in his last email, and ...
Anyway, it would be nice if you avoid introducing the breakage and semantic changes, or at least clearly mark them.
this seems to be our job, not his.
That depends. Fixing incorrect use of iterators is our job, but figuring out what dummy_func(void) is good for is definitely not (ditto for writing autoconf tests for msvc-specific stuff). Best regards, Alexei
On Fri, Apr 17, 2009 at 9:17 AM, Alexei Sheplyakov <varg@metalica.kh.ua> wrote:
Thanks for a patch. Supporting more platforms/compilers is certainly a good thing as long as it doesn't break the code and doesn't introduce subtle changes. Unfortunately your patch *does* introduce subtle changes (and breaks compilation with any non-m$ compiler).
Note that it is not intended to be applied as-is to ginac source tree.
Anyway, it would be nice if you avoid introducing the breakage and semantic changes, or at least clearly mark them.
You know, it took me some time to track down incorrect usage of STL iterators in ginac sources. I tried to fix the ones I could find (because test suite crashed), but I'm sure there are more of them. Yes, it's possible that I introduced other problems, but I have no knowledge of ginac and what it's doing internally, so those fixes are just blind guesses. The patch was more intended to identify problems and propose a naive solution (which at least you know works with MSVC). If you don't want to use it, then don't.
diff -ur ginac-1.5.1/ginac/ncmul.cpp ginac-1.5.1-new/ginac/ncmul.cpp --- ginac-1.5.1/ginac/ncmul.cpp 2009-02-17 13:39:22 +0000 +++ ginac-1.5.1-new/ginac/ncmul.cpp 2009-04-15 23:14:00 +0100 @@ -340,7 +340,7 @@
// determine return types unsignedvector rettypes; - rettypes.reserve(assocseq.size()); + rettypes.resize(assocseq.size());
I'm afraid this change breaks canonicalization of non-commutative products.
I'm afraid that setting vector's capacity doesn't allow you to reference elements outside of the actual vector's size.
Some comments about the patch: - the __alignof__ vs. __alignof could be turned into a configure test
The patch as is breaks compilation with any non-m$ compiler. That's certainly not welcome.
I know it breaks non-VC compiler. That's why I mention the use of a configure test. But I leave that up to you.
- VC++ does not support __func__ macro, so I changed it into __FILE__
Stripping debugging info is not welcome either. Please #ifdef _MSVC (or whatever it is) these changes.
- in exprseq.cpp, I had to add a dummy function to make VC++ instantiate expreseq::info(); don't know why...
Please #ifdef _MSVC that function. Also it would be nice to have a comment in the source explaining why it's necessary (otherwise it's so tempting to delete that pointless code). Other changes, like
diff -ur ginac-1.5.1/ginac/parser/parser.cpp ginac-1.5.1-new/ginac/parser/parser.cpp --- ginac-1.5.1/ginac/parser/parser.cpp 2009-02-17 13:39:22 +0000 +++ ginac-1.5.1-new/ginac/parser/parser.cpp 2009-04-15 20:59:47 +0100 @@ -82,7 +82,7 @@ }
extern numeric* _num_1_p; -extern ex _ex0; +extern const ex _ex0;
need such an #ifdef and a comment too.
Why? The exported symbol uses the "const" modifier. Declaring it non-const like this implicitely assumes that the C++ mangled symbols (const and non-const) will be the same. That's not the case in VC++.
P.S. (How) Did you manage to compile CLN with msvc?
Magic. VC++ per-se is quite (not fully) standard compliant. Most of the problems are in the autotool based build framework. With a bit of coding, you can make VC++ look like GCC. Michael.
Hi! Michael Goffioul wrote:
(How) Did you manage to compile CLN with msvc?
Magic.
VC++ per-se is quite (not fully) standard compliant. Most of the problems are in the autotool based build framework. With a bit of coding, you can make VC++ look like GCC.
Interesting. Could you write up a summary of what you did and report it on <cln-list@ginac.de>? This way, others will have a chance to pick up on your work. Bye -richy. -- Richard B. Kreckel <http://www.ginac.de/~kreckel/>
On Sun, Apr 19, 2009 at 9:53 PM, Richard B. Kreckel <kreckel@ginac.de> wrote:
Hi!
Michael Goffioul wrote:
(How) Did you manage to compile CLN with msvc?
Magic.
VC++ per-se is quite (not fully) standard compliant. Most of the problems are in the autotool based build framework. With a bit of coding, you can make VC++ look like GCC.
Interesting. Could you write up a summary of what you did and report it on <cln-list@ginac.de>? This way, others will have a chance to pick up on your work.
CLN/GiNaC compilation with VC++ is part of the making of Windows package for octave. All compilation scripts and patches are available under SVN on octave-forge http://octave.svn.sourceforge.net/viewvc/octave/trunk/octave-forge/admin/Win... (run_compilation.sh and libs/) The main problem when compiling packages with VC++ is not the compiler itself, which is pretty C/C++ compliant, but the build toolchain. Most packages in the UNIX world use autoconf/automake/libtool toolchain and those tools do not support VC++ very well (at all). But you can work around that problem by using wrappers around VC++ compiler to make it look like GCC, by translating command flags. You can use such wrappers to build tools equivalent to gcc, g++, ar, ranlib... but based on VC++. You can also provide some missing headers like stdint.h, inttypes.h or unistd.h (you can find some on the web), to further ease porting packages. Once you have those tools, it becomes easier to compile packages using the normal "./configure && make" way, using the MSYS shell. But you still need to take care of some specific issues: 1) libtool: using some post-processing magic, it's possible to make libtool to support VC++ (through the gcc-like wrappers) 2) symbol export: VC++ does not export symbols automatically, so you need to take of it; either by using a .def file (some packages provide their own), or by using dllexport/dllimport qualifiers (again some packages support that as well), or by parsing yourself the object files to generate a .def file automatically (this works well especially for plain C code); of course, when you build a static library (which is what I do or CLN/GiNaC), you don't have to care about that 3) export of data: even with the tricks of point 2), you still need to decorate exported data with dllimport, as VC++ does not have the auto-import feature of MinGW; for packages that do not provide this decoration, you have to write your own patch. For the specific case of CLN, there were a couple of issues I also had to take care of: - struct/class, const/non-const: at some places, CLN foward declares objects as being "class", while there are actually "struct"; VC++ generates different mangled names for struct and class, so you end up with unresolved references. The same happen for const/non-const (forward declare as non-const, but define as const). - assembly code: here I cheated a little bit and used gcc to compile the .S file; this required some hacks in the Makefile - some namespace issues: apparently, declaring a function inside the body of another method, which is parted of the cln namespace, does not make VC++ to tag the declared function with the cln namespace; for instance in number.h, I had to move the "extern" statements of CL_DEFINE_LONG_CONSTRUCTOR out of the method bodies. Bye, Michael.
Hi! On Sun, Apr 19, 2009 at 11:26:26PM +0100, Michael Goffioul wrote:
The main problem when compiling packages with VC++ is not the compiler itself, which is pretty C/C++ compliant,
Actually CLN itself is not quite standard compliant. It used to rely on non-standard compiler and linker features, such as GNU inline semantics (which is IMHO much better than one imposed by C++ and C99), weak symbols support, etc. Although most of these issues are already fixed there are definitely quite a number of GCC'isms left. [skipped auto* tools rant]
For the specific case of CLN, there were a couple of issues I also had to take care of: objects as being "class", while there are actually "struct"; VC++ generates different mangled names for struct and class, so you end up with unresolved references.
AFAIK struct and class are synonymous, the standard says: \begin{quote} [class] 4 A structure is a class defined with the class-key struct; its members and base classes (clause 10) are public by default (clause 11). \end{quote} Anyway, if your compiler doesn't care about standards, please submit a patch. Note: please take care to write proper comments (i.e. why patch is necessary at all, what problem[s] it solves, etc.) and not break compilation on a primary platform (GNU/Linux). Also it would be nice if you separate unrelated changes and post them as individual patches (as opposed to all-in-one patch).
The same happen for const/non-const (forward declare as non-const, but define as const).
Very interesting. [basic.type.qualifier] says: \begin{quote} The cv-qualified or cv-unqualified versions of a type are distinct types; however, they shall have the same representation and alignment requirements (3.9)^46) 46) The same representation and alignment requirements are meant to imply interchangeability as arguments to functions, return values from functions, and members of unions. \end{quote} I don't quite understand how it's possible for cv-qualified and non-cv-qualified type to have different mangled names without violating the above requirements. Anyway, please submit a patch.
- assembly code: here I cheated a little bit and used gcc to compile the .S file; this required some hacks in the Makefile
- some namespace issues: apparently, declaring a function inside the body of another method, which is parted of the cln namespace, does not make VC++ to tag the declared function with the cln namespace;
This is definitely a compiler bug.
for instance in number.h, I had to move the "extern" statements of CL_DEFINE_LONG_CONSTRUCTOR out of the method bodies.
I'm afraid such an approach is unacceptable. Those functions are intended for internal usage only, and they are hidden from users for a reason (otherwise it would be very difficult to guarantee binary compatibility). We could export them only for msvc, but then we can't guarantee binary (and source-level) compatibility any more. Best regards, Alexei
On Tue, Apr 21, 2009 at 9:02 AM, Alexei Sheplyakov <varg@metalica.kh.ua> wrote:
The same happen for const/non-const (forward declare as non-const, but define as const).
Very interesting. [basic.type.qualifier] says:
\begin{quote} The cv-qualified or cv-unqualified versions of a type are distinct types; however, they shall have the same representation and alignment requirements (3.9)^46)
46) The same representation and alignment requirements are meant to imply interchangeability as arguments to functions, return values from functions, and members of unions. \end{quote}
I don't quite understand how it's possible for cv-qualified and non-cv-qualified type to have different mangled names without violating the above requirements.
Anyway, please submit a patch.
Sorry, I won't. I don't see why I would help your project(s), given the recent GPL violation claims. Michael.
On Tue, Apr 21, 2009 at 09:17:18AM +0100, Michael Goffioul wrote:
Anyway, please submit a patch.
Sorry, I won't. I don't see why I would help your project(s),
Those patches are of no help for me -- I don't use msvc (or any other m$' software). However, they might be useful for windows users who can't use MinGW toolchain (for whatever reason). Please re-consider. [skipped legalistic nonsense] Best regards, Alexei
On Thu, Apr 16, 2009 at 8:43 AM, Michael Goffioul <michael.goffioul@gmail.com> wrote:
On Thu, Apr 16, 2009 at 2:04 PM, Jens Vollinga <jensv@nikhef.nl> wrote:
I'm no C++ standard expert, but when I look at http://www.sgi.com/tech/stl/RandomAccessIterator.html my understanding of the precondition for "i += n" is that you cannot go after end().
It seems that the standard (I only have the draft. Hopefully there is no difference to the final version in the areas under consideration) and the SGI documentation deviate from each other. In SGI there are pre- and postconditions on +, -, +=, and -=. And these conditions are violated in the examples as you point out. But in the standard +, -, +=, -= have NO pre-/postconditions to them! See 24.1.5. The confusing part is, that -- and ++ DO have such conditions. But for the ++ operation, it is explicitly allowed to go past-the-end. See 24.1.4. So no problem for the code in factor.cpp. The code in utils.h seems to be against the standard, though, but I am no language lawyer and so I am maybe misinterpreting the conditions there. In case, this can be fixed by using reverse iterators.
OK, I had a look in a draft C++ standard document I could find on the web, and when reading 24.1.3, the "pre" condition for ++r is that r must be dereferenceable. When r == end(), it is not dereferenceable, so you cannot do ++r. Don't you have the same interpretation?
Yes. -- Gaby
Jens Vollinga wrote:
Hi,
Michael Goffioul schrieb:
I'm currently tracking down these issues by running the test suite in the source package, but this might not catch all of them. If you are interested, I can send a patch once I succeed to run the test suite completely.
yes, that would be interesting! I'd like to see where this happens. I don't have access to a VC compiler to try it myself, though.
I see no problem with dereferencing end(), and doing operations like end()+<integer>. In that case, end() should return a proper iterator as a temporary and on these the operations +,- are defined (somewhere 24.1.5 in the standard, I believe). But maybe there is a ++ or -- involved?
As far as I recall, end() cannot be dereferenced, but only used to test for out-of-range. In any case, to track problems run-time with libstdc++, you can enable specific debug mode (see <http://gcc.gnu.org/onlinedocs/libstdc++/manual/debug_mode.html>, thanks to Marco Morandini for pointing it out). I do not recall any compile-time tests available with g++. Cheers, p.
Hi, Pierangelo Masarati schrieb:
I see no problem with dereferencing end(), and doing operations like end()+<integer>. In that case, end() should return a proper iterator as a temporary and on these the operations +,- are defined (somewhere 24.1.5 in the standard, I believe). But maybe there is a ++ or -- involved?
As far as I recall, end() cannot be dereferenced, but only used to test for out-of-range. In any case, to track problems run-time with
yes, sorry, you're right. The dereferencing is of course not guaranteed to work. But the arithmetic is nevertheless allowed (at least officially). I mixed up that sentence, because in the standard the conditions on the operations ++,--,etc. involving these iterators are often defined using the notion of dereferencing. For the ++ operation there is an exemption to the requirement for iterators to be dereferenceable. Regards, Jens
Hi, On Thu, Apr 16, 2009 at 02:34:18PM +0200, Pierangelo Masarati wrote:
As far as I recall, end() cannot be dereferenced, but only used to test for out-of-range. In any case, to track problems run-time with libstdc++, you can enable specific debug mode (see <http://gcc.gnu.org/onlinedocs/libstdc++/manual/debug_mode.html>, thanks to Marco Morandini for pointing it out).
Thanks for a tip. I've reproduced the problem, and working on fixing it. Best regards, Alexei
Hi, Michael Goffioul schrieb:
I'm currently tracking down these issues by running the test suite in the source package, but this might not catch all of them. If you are interested, I can send a patch once I succeed to run the test suite completely.
a fast grep revealed only two fishy lines of code. Both in inifcns_nstdsums.cpp (lines 2458 and 3280, master branch). Maybe the attached patch can solve the problems with VC++? Regards, Jens P.S.: this mailing list takes ages to send the mails at the moment ... diff --git a/ginac/inifcns_nstdsums.cpp b/ginac/inifcns_nstdsums.cpp index 2c4061a..94fd9ce 100644 --- a/ginac/inifcns_nstdsums.cpp +++ b/ginac/inifcns_nstdsums.cpp @@ -2455,7 +2455,8 @@ lst convert_parameter_Li_to_H(const lst& m, const lst& x, ex& pf) { lst res; lst::const_iterator itm = m.begin(); - lst::const_iterator itx = ++x.begin(); + lst::const_iterator itx = x.begin(); + ++itx; int signum = 1; pf = _ex1; res.append(*itm); @@ -3277,7 +3278,8 @@ static ex H_eval(const ex& m_, const ex& x) pos1 = *m.begin(); p = _ex1; } - for (lst::const_iterator it = ++m.begin(); it != m.end(); it++) { + lst::const_iterator it = m.begin(); + for (++it; it != m.end(); ++it) { if ((*it).info(info_flags::integer)) { if (step == 0) { if (*it > _ex1) {
Hi, On Thu, Apr 16, 2009 at 01:42:23PM +0200, Jens Vollinga wrote:
diff --git a/ginac/inifcns_nstdsums.cpp b/ginac/inifcns_nstdsums.cpp index 2c4061a..94fd9ce 100644 --- a/ginac/inifcns_nstdsums.cpp +++ b/ginac/inifcns_nstdsums.cpp @@ -2455,7 +2455,8 @@ lst convert_parameter_Li_to_H(const lst& m, const lst& x, ex& pf) { lst res; lst::const_iterator itm = m.begin(); - lst::const_iterator itx = ++x.begin(); + lst::const_iterator itx = x.begin(); + ++itx;
As far as I understand both versions are completely equivalent, aren't they?
int signum = 1; pf = _ex1; res.append(*itm); @@ -3277,7 +3278,8 @@ static ex H_eval(const ex& m_, const ex& x) pos1 = *m.begin(); p = _ex1; } - for (lst::const_iterator it = ++m.begin(); it != m.end(); it++) { + lst::const_iterator it = m.begin(); + for (++it; it != m.end(); ++it) {
Same here (the iterator is incremented before testing for != end()). Best regards, Alexei
Hi, Alexei Sheplyakov schrieb:
+ lst::const_iterator itx = x.begin(); + ++itx;
As far as I understand both versions are completely equivalent, aren't they?
of course they are. You better ignore all emails I sent on that day to the mailing list. They contain mostly nonsense. :-) Except for these lines: Jens Vollinga schrieb:
this seems to be our job, not his.
So, the question is: will you care about this or should I? We can discuss this on the devel-list if you like.
Regards, Jens
participants (8)
-
Alexei Sheplyakov
-
Gabriel Dos Reis
-
James Jackson
-
Jens Vollinga
-
Michael Abshoff
-
Michael Goffioul
-
Pierangelo Masarati
-
Richard B. Kreckel