[SCM] GiNaC -- a C++ library for symbolic computations branch, master, updated. release_1-4-0-708-ga569b474
This is an automated email from the git hooks/post-receive script. It was generated because a ref change was pushed to the repository containing the project "GiNaC -- a C++ library for symbolic computations". The branch, master has been updated via a569b4748e266ce318602e7b6a9c1f19a8c75bdf (commit) via c3195f0b5a7ac9fdbfdd04e5f4acf6a836063de0 (commit) from 79f30c335f1ddbd3c76dfee5d76128b992b6b19c (commit) Those revisions listed above that are new to this repository have not appeared on any other notification email; so we list those revisions in full, below. - Log ----------------------------------------------------------------- commit a569b4748e266ce318602e7b6a9c1f19a8c75bdf Author: Alexey Sheplyakov <asheplyakov@altlinux.org> Date: Fri Jan 1 20:09:47 2021 +0400 bugfix: function: always pass evalf'ed arguments to evalf_funcp GiNaC passes unevaluted arguments to a function with evalf_funcp taking an exvector. Fixed that, and added a test case commit c3195f0b5a7ac9fdbfdd04e5f4acf6a836063de0 Author: Alexey Sheplyakov <asheplyakov@altlinux.org> Date: Wed Jan 6 11:20:46 2021 +0400 Avoid multiple definitions of `lst::info` (MinGW compile fix) [55/59] Linking CXX shared library bin/libginac.dll FAILED: bin/libginac.dll ginac/libginac.dll.a [skipped long list of object files] /usr/bin/x86_64-w64-mingw32-ld: ginac/CMakeFiles/ginac.dir/lst.cpp.obj: in function `GiNaC::ptr<GiNaC::basic>::~ptr()': /home/asheplyakov/work/sw/ginac/_build_w64/../ginac/container.h:150: multiple definition of `GiNaC::container<std::__cxx11::list>::info(unsigned int) const'; ginac/CMakeFiles/ginac.dir/integration_kernel.cpp.obj:/home/asheplyakov/work/sw/ginac/_build_w64/../ginac/container.h:116: first defined here integration_kernel.cpp makes use of GiNaC::lst without including the `lst.h` header. That's possible since there's a typedef in `registrar.h` (included by virtually all GiNaC sources) and a defintion in `container.h` (included via `add.h` -> `expairseq.h` -> `indexed.h` -> `exprseq.h`), so the compiler can instantiate container<std::list>. However the explicit specialization of `lst::info` is not available (in integration_kernel.cpp). This violates 17.8.3.6 [templ.expl.spec] which demands If a template, a member template or a member of a class template is explicitly specialized then that specialization shall be declared before the first use of that specialization that would cause an implicit instantiation to take place, in every translation unit in which such a use occurs; no diagnostic is required. If the program does not provide a definition for an explicit specialization and either the specialization is used ina way that would cause an implicit instantiation to take place or the member is a virtual member function, the program is ill-formed, no diagnostic required. On ELF platforms libginac appears to link just fine despite having two instantiations of `lst::info` since the of them (in `integration_kernel.o`) is a weak symbol: $ find ginac -type f -name '*.o' | xargs -n 1 nm --print-file-name --defined | c++filt | grep -e 'list>::info(' ginac/CMakeFiles/ginac.dir/integration_kernel.cpp.o:0000000000000000 W GiNaC::container<std::__cxx11::list>::info(unsigned int) const ginac/CMakeFiles/ginac.dir/lst.cpp.o:0000000000000000 T GiNaC::container<std::__cxx11::list>::info(unsigned int) const so the linker discards the wrong instantiation of `lst::info` method. However on MinGW there are no weak symbols (in ELF sense): $ find ginac -type f -name '*.obj' | xargs -n 1 x86_64-w64-mingw32-nm --print-file-name --defined | c++filt | grep -e 'list>::info(' ginac/CMakeFiles/ginac.dir/lst.cpp.obj:0000000000000010 T GiNaC::container<std::__cxx11::list>::info(unsigned int) const ginac/CMakeFiles/ginac.dir/integration_kernel.cpp.obj:0000000000000000 T GiNaC::container<std::__cxx11::list>::info(unsigned int) const Hence the above multiple definition error. To avoid the problem #include "lst.h" (so explicit specialization is available). While at it explicitly instantiate lst::info method in lst.cpp A better solution would be to remove declaration of lst from registrar.h, but that's too disruptive since GiNaC uses lst a lot: subs, unarchive, etc. ----------------------------------------------------------------------- Summary of changes: check/CMakeLists.txt | 4 +- check/Makefile.am | 4 ++ check/exam_function_exvector.cpp | 116 +++++++++++++++++++++++++++++++++++++++ ginac/function.cppy | 2 +- ginac/integration_kernel.h | 1 + ginac/lst.cpp | 1 + ginac/lst.h | 1 + 7 files changed, 127 insertions(+), 2 deletions(-) create mode 100644 check/exam_function_exvector.cpp hooks/post-receive -- GiNaC -- a C++ library for symbolic computations
Alexey, Is that patch really correct? On 06.01.21 16:45, Richard B. Kreckel via GiNaC-devel wrote:
commit c3195f0b5a7ac9fdbfdd04e5f4acf6a836063de0 Author: Alexey Sheplyakov <asheplyakov@altlinux.org> Date: Wed Jan 6 11:20:46 2021 +0400
Avoid multiple definitions of `lst::info` (MinGW compile fix)
[55/59] Linking CXX shared library bin/libginac.dll FAILED: bin/libginac.dll ginac/libginac.dll.a [skipped long list of object files] /usr/bin/x86_64-w64-mingw32-ld: ginac/CMakeFiles/ginac.dir/lst.cpp.obj: in function `GiNaC::ptr<GiNaC::basic>::~ptr()': /home/asheplyakov/work/sw/ginac/_build_w64/../ginac/container.h:150: multiple definition of `GiNaC::container<std::__cxx11::list>::info(unsigned int) const'; ginac/CMakeFiles/ginac.dir/integration_kernel.cpp.obj:/home/asheplyakov/work/sw/ginac/_build_w64/../ginac/container.h:116: first defined here
integration_kernel.cpp makes use of GiNaC::lst without including the `lst.h` header. That's possible since there's a typedef in `registrar.h` (included by virtually all GiNaC sources) and a defintion in `container.h` (included via `add.h` -> `expairseq.h` -> `indexed.h` -> `exprseq.h`), so the compiler can instantiate container<std::list>. However the explicit specialization of `lst::info` is not available (in integration_kernel.cpp). This violates 17.8.3.6 [templ.expl.spec] which demands
If a template, a member template or a member of a class template is explicitly specialized then that specialization shall be declared before the first use of that specialization that would cause an implicit instantiation to take place, in every translation unit in which such a use occurs; no diagnostic is required. If the program does not provide a definition for an explicit specialization and either the specialization is used ina way that would cause an implicit instantiation to take place or the member is a virtual member function, the program is ill-formed, no diagnostic required.
On ELF platforms libginac appears to link just fine despite having two instantiations of `lst::info` since the of them (in `integration_kernel.o`) is a weak symbol:
$ find ginac -type f -name '*.o' | xargs -n 1 nm --print-file-name --defined | c++filt | grep -e 'list>::info(' ginac/CMakeFiles/ginac.dir/integration_kernel.cpp.o:0000000000000000 W GiNaC::container<std::__cxx11::list>::info(unsigned int) const ginac/CMakeFiles/ginac.dir/lst.cpp.o:0000000000000000 T GiNaC::container<std::__cxx11::list>::info(unsigned int) const
so the linker discards the wrong instantiation of `lst::info` method. However on MinGW there are no weak symbols (in ELF sense):
$ find ginac -type f -name '*.obj' | xargs -n 1 x86_64-w64-mingw32-nm --print-file-name --defined | c++filt | grep -e 'list>::info(' ginac/CMakeFiles/ginac.dir/lst.cpp.obj:0000000000000010 T GiNaC::container<std::__cxx11::list>::info(unsigned int) const ginac/CMakeFiles/ginac.dir/integration_kernel.cpp.obj:0000000000000000 T GiNaC::container<std::__cxx11::list>::info(unsigned int) const
Hence the above multiple definition error.
To avoid the problem #include "lst.h" (so explicit specialization is available). While at it explicitly instantiate lst::info method in lst.cpp
A better solution would be to remove declaration of lst from registrar.h, but that's too disruptive since GiNaC uses lst a lot: subs, unarchive, etc.
I came across it because clang++ now complains: lst.cpp:41:37: warning: explicit instantiation of 'info' that occurs after an explicit specialization has no effect [-Winstantiation-after-specialization] Wouldn't it be sufficent to include "lst.h" in "integration_kernel.cpp"? I must admit that I do not understand the rest of it. All my best, -richy. -- Richard B. Kreckel <https://in.terlu.de/~kreckel/>
Richard, On 2/3/21 6:59 PM, Richard B. Kreckel wrote:
Is that patch really correct?
On 06.01.21 16:45, Richard B. Kreckel via GiNaC-devel wrote:
commit c3195f0b5a7ac9fdbfdd04e5f4acf6a836063de0 Author: Alexey Sheplyakov <asheplyakov@altlinux.org> Date: Wed Jan 6 11:20:46 2021 +0400
There's a mistake in the commit message: the section of the standard which describes the explicit template specialization is 14.7.3. Everything else looks fine for me.
I came across it because clang++ now complains:
lst.cpp:41:37: warning: explicit instantiation of 'info' that occurs after an explicit specialization has no effect [-Winstantiation-after-specialization]
I don't quite understand the warning. What's the point of the explicit specialization **after** the thing has been already instantiated (either explicitly or implicitly)? And having an explicit specialization *after* the method has been implicitly instantiated is an error. I'll re-read 14.7 ("Template instantiation and specialization") once more, but for now I think clang++ is wrong, and we should ignore the warning. I.e. put something like this into lst.cpp: // ignore bogus "explicit instantiation that occurs after an explicit specialization" warning #if defined(__clang__) && __has_warning("-Winstantiation-after-specialization") #pragma clang diagnostic ignored "-Winstantiation-after-specialization" #endif
Wouldn't it be sufficent to include "lst.h" in "integration_kernel.cpp"?
I think it's implementation defined. The definition of lst::info() is available in lst.cpp only, and nothing in that translation unit uses lst::info(). Therefore the compiler is not obliged to instantiate lst::info() (although it's OK to do so). gcc and clang seem to instantiate it, but msvc needs the explicit instantiation (or something which triggers the implicit instantiation, like `dummy_func` in exprseq.cpp) Best regards, Alexey
Alexey, On 08.02.21 11:10, Alexey Sheplyakov wrote:
I think it's implementation defined. The definition of lst::info() is available in lst.cpp only, and nothing in that translation unit uses lst::info(). Therefore the compiler is not obliged to instantiate lst::info() (although it's OK to do so). gcc and clang seem to instantiate it, but msvc needs the explicit instantiation (or something which triggers the implicit instantiation, like `dummy_func` in exprseq.cpp)
If we just provided an inlined definition lst::info() in lst.h, all compilers should be happy, right? -richy. -- Richard B. Kreckel <https://in.terlu.de/~kreckel/>
Dear Alexey, I now tried building with MinGW-w64. On 08.02.21 11:10, Alexey Sheplyakov wrote:
There's a mistake in the commit message: the section of the standard which describes the explicit template specialization is 14.7.3. Everything else looks fine for me.
I came across it because clang++ now complains:
lst.cpp:41:37: warning: explicit instantiation of 'info' that occurs after an explicit specialization has no effect [-Winstantiation-after-specialization]
I don't quite understand the warning. What's the point of the explicit specialization **after** the thing has been already instantiated (either explicitly or implicitly)? And having an explicit specialization *after* the method has been implicitly instantiated is an error. I'll re-read 14.7 ("Template instantiation and specialization") once more, but for now I think clang++ is wrong, and we should ignore the warning. I.e. put something like this into lst.cpp:
Okay, it is indeed weird. CLang++ complains about your explicit instantiation in lst.cpp: template bool container<std::list>::info(unsigned) const; saying that the previous template specialization is in lst.hpp: template<> bool lst::info(unsigned inf) const; which is merely a declaration. So, CLang++ seems to miss the entire the point of the explicit instantiation.
// ignore bogus "explicit instantiation that occurs after an explicit specialization" warning #if defined(__clang__) && __has_warning("-Winstantiation-after-specialization") #pragma clang diagnostic ignored "-Winstantiation-after-specialization" #endif
Wouldn't it be sufficent to include "lst.h" in "integration_kernel.cpp"?
For the record: #including "lst.h" in "integration_kernel.cpp" as your patch does is indeed enough for building with MinGW64.
I think it's implementation defined. The definition of lst::info() is available in lst.cpp only, and nothing in that translation unit uses lst::info(). Therefore the compiler is not obliged to instantiate lst::info() (although it's OK to do so). gcc and clang seem to instantiate it, but msvc needs the explicit instantiation (or something which triggers the implicit instantiation, like `dummy_func` in exprseq.cpp)
I would like to keep the lst.* and exprseq.* similar to make it easier to read the code. (I had quite a hard time reading it.) So I consider replacing the explicit instantiation in lst.* with the dummy_func() trick in exprseq.*, or remove it altogether. (Do recent versions of MSVC still need it?) Best wishes, -richy. -- Richard B. Kreckel <https://in.terlu.de/~kreckel/>
Dear Richard, On 2/22/21 12:12 AM, Richard B. Kreckel wrote:
Okay, it is indeed weird.
CLang++ complains about your explicit instantiation in lst.cpp: template bool container<std::list>::info(unsigned) const; saying that the previous template specialization is in lst.hpp: template<> bool lst::info(unsigned inf) const; which is merely a declaration.
So, CLang++ seems to miss the entire the point of the explicit instantiation.
Let's put it this way: either clang++ or /me misses the entire point of the explicit instantiation.
// ignore bogus "explicit instantiation that occurs after an explicit specialization" warning #if defined(__clang__) && `>> __has_warning("-Winstantiation-after-specialization") #pragma clang diagnostic ignored "-Winstantiation-after-specialization" #endif
Wouldn't it be sufficent to include "lst.h" in "integration_kernel.cpp"?
For the record: #including "lst.h" in "integration_kernel.cpp" as your patch does is indeed enough for building with MinGW64.
That's a good news. However it's still implementation defined.
I think it's implementation defined. The definition of lst::info() is available in lst.cpp only, and nothing in that translation unit uses lst::info(). Therefore the compiler is not obliged to instantiate lst::info() (although it's OK to do so). gcc and clang seem to instantiate it, but msvc needs the explicit instantiation (or something which triggers the implicit instantiation, like `dummy_func` in exprseq.cpp)
I would like to keep the lst.* and exprseq.* similar to make it easier to read the code. (I had quite a hard time reading it.)
/me too. In fact I've got a patch which adds an explicit instantiation of exprseq::info() and removes dummy_func(). I haven't had a chance to test it with msvc yet.
So I consider replacing the explicit instantiation in lst.* with the dummy_func() trick in exprseq.*, or remove it altogether. (Do recent versions of MSVC still need it?)
I don't quite like this idea. Let's keep the explicit instantiations, and suppress clang++ warnings for now. Making lst::info inline does not sound like a good idea either. For one any change in inline function typically requires SONAME bump [1], so there should be a pretty good reason to make a function/method inline. Usually it's the performance, not a (bogus) compiler warning. [1] SONAME bump is NOT required if it's safe for programs linked with the prior version of the library to call the old implementation. However it's tricky and might be dangerous. Best regards, Alexey
Dear Alexey, On 22.02.21 19:19, Alexey Sheplyakov wrote:
/me too. In fact I've got a patch which adds an explicit instantiation of exprseq::info() and removes dummy_func(). I haven't had a chance to test it with msvc yet.
Wouldn't it be just the same as in lst.*? That shouldn't need much testing unless MSVC suffers borderline personality disorder, IMHO. In any case, these lines of code can benefit from a good comment! (Case in point: I was scratching my head way too long over this stuff.)
So I consider replacing the explicit instantiation in lst.* with the dummy_func() trick in exprseq.*, or remove it altogether. (Do recent versions of MSVC still need it?)
I don't quite like this idea. Let's keep the explicit instantiations, and suppress clang++ warnings for now.
Oh, I don't care very much about this compiler warning unless there is something we can improve. (A #pragma isn't an improvement, I'ld say.)
Making lst::info inline does not sound like a good idea either. For one any change in inline function typically requires SONAME bump [1], so there should be a pretty good reason to make a function/method inline. Usually it's the performance, not a (bogus) compiler warning.
Okay. -richy. -- Richard B. Kreckel <https://in.terlu.de/~kreckel/>
participants (3)
-
Alexey Sheplyakov
-
Richard B. Kreckel
-
Richard B. Kreckel