Re: [Patch] Refractor Thompson matcher
Hi,
On 07/31/2013 05:43 AM, Tim Shen wrote:
I found some other wired problems in that patch after committing.
Probably need more time to work on it, so now I revert it :(
in order to bootstrap successfully the patch I needed these additional
changes. Please double check at your end, run the testsuite, and if
everything goes well, please commit (again ;) the whole thing.
Thanks,
Paolo.
Index: src/c++11/functexcept.cc
===
--- src/c++11/functexcept.cc(revision 201363)
+++ src/c++11/functexcept.cc(working copy)
@@ -30,7 +30,7 @@
#include system_error
#include future
#include functional
-#include regex
+#include bits/regex_error.h
#ifdef _GLIBCXX_USE_NLS
# include libintl.h
Index: src/c++11/regex.cc
===
--- src/c++11/regex.cc (revision 201363)
+++ src/c++11/regex.cc (working copy)
@@ -22,7 +22,8 @@
// see the files COPYING3 and COPYING.RUNTIME respectively. If not, see
// http://www.gnu.org/licenses/.
-#include regex
+#include stdexcept
+#include bits/regex_error.h
namespace std _GLIBCXX_VISIBILITY(default)
{
Re: [Patch] Refractor Thompson matcher
On Wed, Jul 31, 2013 at 8:18 PM, Paolo Carlini paolo.carl...@oracle.com wrote: in order to bootstrap successfully the patch I needed these additional changes. Please double check at your end, run the testsuite, and if everything goes well, please commit (again ;) the whole thing. Fully tested: cd gcc-bulid rm -r * ../gcc/configure --enable-languages=c++ make bootstrap make -k check ...and committed(r201391). -- Tim Shen
Re: [Patch] Refractor Thompson matcher
On Mon, Jul 29, 2013 at 4:26 PM, Paolo Carlini paolo.carl...@oracle.com wrote: Ah, excellent. As usual, let's wait a day or so for further comments and then please commit the whole thing. Commited. -- Tim Shen
Re: [Patch] Refractor Thompson matcher
On 07/30/2013 02:07 PM, Tim Shen wrote: On Mon, Jul 29, 2013 at 4:26 PM, Paolo Carlini paolo.carl...@oracle.com wrote: Ah, excellent. As usual, let's wait a day or so for further comments and then please commit the whole thing. Commited. The bootstrap is currently broken due to this commit. I'm quickly moving inline a few functions in the *non* template _BFSMatcher which were out of line. When you consider the design stable enough please take care of exporting from the *.so the large non-template functions. Thanks, Paolo.
Re: [Patch] Refractor Thompson matcher
.. no, too many changes, please simply revert the commit ASAP and next time please test more carefully before posting. Thanks, Paolo.
Re: [Patch] Refractor Thompson matcher
Hi again, I reverted the commit and tested that mainline is fine again. Just to clarify how we normally handle these issues in v3: *temporarily*, to avoid the linkage issues which broke the bootstrap today, all the non-template functions must be inline, even if large. In the past normally we had the definitions of such functions in *.tcc files, thus explicitly inline, with FIXME comments. I think it makes sense to do this for regex too. In any case, as soon as the design stabilizes all such functions shall be exported by the dynamic library, thus in practice all the definitions will be moved to src/c++11/*.cc files. Thanks, Paolo
Re: [Patch] Refractor Thompson matcher
On Wed, Jul 31, 2013 at 6:10 AM, Paolo Carlini paolo.carl...@oracle.com wrote: I reverted the commit and tested that mainline is fine again. Sorry for the accident! Just to clarify how we normally handle these issues in v3: *temporarily*, to avoid the linkage issues which broke the bootstrap today, all the non-template functions must be inline, even if large. In the past normally we had the definitions of such functions in *.tcc files, thus explicitly inline, with FIXME comments. I think it makes sense to do this for regex too. In any case, as soon as the design stabilizes all such functions shall be exported by the dynamic library, thus in practice all the definitions will be moved to src/c++11/*.cc files. I see. So I include regex in different files and then compile them together, it broke. I've make every non-templated function in this commit inline. Now it compiles. Sorry again for this commit. Should I commit again? -- Tim Shen
Re: [Patch] Refractor Thompson matcher
On Wed, Jul 31, 2013 at 6:44 AM, Tim Shen timshe...@gmail.com wrote: On Wed, Jul 31, 2013 at 6:10 AM, Paolo Carlini paolo.carl...@oracle.com wrote: I reverted the commit and tested that mainline is fine again. Sorry for the accident! Just to clarify how we normally handle these issues in v3: *temporarily*, to avoid the linkage issues which broke the bootstrap today, all the non-template functions must be inline, even if large. In the past normally we had the definitions of such functions in *.tcc files, thus explicitly inline, with FIXME comments. I think it makes sense to do this for regex too. In any case, as soon as the design stabilizes all such functions shall be exported by the dynamic library, thus in practice all the definitions will be moved to src/c++11/*.cc files. I see. So I include regex in different files and then compile them together, it broke. I've make every non-templated function in this commit inline. Now it compiles. Sorry again for this commit. Should I commit again? -- Tim Shen -- Tim Shen inline.patch Description: Binary data
Re: [Patch] Refractor Thompson matcher
Hi, On 07/31/2013 12:44 AM, Tim Shen wrote: I see. So I include regex in different files and then compile them together, it broke. I've make every non-templated function in this commit inline. Now it compiles. Sorry again for this commit. Did you actually bootstrap test the patch? Because you didn't last time, right? Should I commit again? Please add FIXME comments to all the non-template functions in *.tcc files (which will be moved to *.cc files). To repeat: the inlines in *.tcc files must be all and only non-templates, as a temporary hack. Thanks, Paolo.
Re: [Patch] Refractor Thompson matcher
On Wed, Jul 31, 2013 at 7:03 AM, Paolo Carlini paolo.carl...@oracle.com wrote: Did you actually bootstrap test the patch? Because you didn't last time, right? I compiled it and run the 28_regex testsuite, nothing wrong happened because there're all single-file testcases in it; but I ignored the duplicated definition problem. It'll never happen in the future. Sorry again. Please add FIXME comments to all the non-template functions in *.tcc files (which will be moved to *.cc files). To repeat: the inlines in *.tcc files must be all and only non-templates, as a temporary hack. Added. -- Tim Shen
Re: [Patch] Refractor Thompson matcher
Hi, On 07/31/2013 01:11 AM, Tim Shen wrote: On Wed, Jul 31, 2013 at 7:03 AM, Paolo Carlini paolo.carl...@oracle.com wrote: Did you actually bootstrap test the patch? Because you didn't last time, right? I compiled it and run the 28_regex testsuite, nothing wrong happened because there're all single-file testcases in it; but I ignored the duplicated definition problem. It'll never happen in the future. Sorry again. Earlier, even without bootstrapping you should have noticed that the library didn't build. It's not about excuses, it's about following a policy, which, should be obvious now, we are following for a reason: all posted patches must be boostrapped tested. Please add FIXME comments to all the non-template functions in *.tcc files (which will be moved to *.cc files). To repeat: the inlines in *.tcc files must be all and only non-templates, as a temporary hack. Added. Please post the complete patch you intend to commit. Part of the GCC policy is also that all the patches must be posted complete, exactly as would be committed upon approval. Paolo.
Re: [Patch] Refractor Thompson matcher
On Wed, Jul 31, 2013 at 7:18 AM, Paolo Carlini paolo.carl...@oracle.com wrote: Please post the complete patch you intend to commit. Part of the GCC policy is also that all the patches must be posted complete, exactly as would be committed upon approval. Here it is. -- Tim Shen bfs.patch Description: Binary data
Re: [Patch] Refractor Thompson matcher
On 07/31/2013 01:26 AM, Tim Shen wrote: On Wed, Jul 31, 2013 at 7:18 AM, Paolo Carlini paolo.carl...@oracle.com wrote: Please post the complete patch you intend to commit. Part of the GCC policy is also that all the patches must be posted complete, exactly as would be committed upon approval. Here it is. Ok, thanks. Normally the ChangeLog entries too are wrapped to 80 columns. Thanks again, Paolo.
Re: [Patch] Refractor Thompson matcher
I found some other wired problems in that patch after committing. Probably need more time to work on it, so now I revert it :( -- Tim Shen
Re: [Patch] Refractor Thompson matcher
Hi, On 07/29/2013 02:06 AM, Tim Shen wrote: On Mon, Jul 29, 2013 at 1:08 AM, Paolo Carlini paolo.carl...@oracle.com wrote: Oh well, thanks. But then I expect specific testcases to come with it, for the various values of the parameter, both the default and the other other values. Otherwise, the idea isn't really immediately useful. See what I mean? So I modified testcases to test both of them. Also, please make sure that this change is in any case a step in the right direction. I mean: I suppose that if we do this only the logic to control automatically the switch is missing and when we add it we are done. Let's not do it if I'm mistaken. Never mind, this patch makes them switch to be automatic(based on whether there're back-references in the regex), thanks to the full-featured regex scanner. A testcase is given too. Ah, excellent. As usual, let's wait a day or so for further comments and then please commit the whole thing. Minor nit: it's not clear to me why in the previous patch you added the includes of map and queue. Thanks, Paolo.
Re: [Patch] Refractor Thompson matcher
On Mon, Jul 29, 2013 at 4:26 PM, Paolo Carlini paolo.carl...@oracle.com wrote: Minor nit: it's not clear to me why in the previous patch you added the includes of map and queue. I use them in regex_grep_matcher.h and regex_grep_matcher.tcc; Is include/std/regex the right place where I include them? -- Tim Shen
Re: [Patch] Refractor Thompson matcher
On 07/29/2013 10:37 AM, Tim Shen wrote: On Mon, Jul 29, 2013 at 4:26 PM, Paolo Carlini paolo.carl...@oracle.com wrote: Minor nit: it's not clear to me why in the previous patch you added the includes of map and queue. I use them in regex_grep_matcher.h and regex_grep_matcher.tcc; Is include/std/regex the right place where I include them? I guess it's the right place yes. Sorry, I didn't notice the uses (when I think refactoring, I don't think two more std:: containers are needed ;) Paolo.
Re: [Patch] Refractor Thompson matcher
Hi, On 07/28/2013 06:13 AM, Tim Shen wrote: Refractor the whole Thompson matcher using the queue-based(BFS) Bellman-Ford algorithm. Fix the grouping problem. Refactor, refactoring, etc, no 'r'. If the grouping problem is now fixed, would it make sense to add corresponding testcases? Paolo.
Re: [Patch] Refractor Thompson matcher
On Sun, Jul 28, 2013 at 5:12 PM, Paolo Carlini paolo.carl...@oracle.com wrote: Refactor, refactoring, etc, no 'r'. Thanks :) If the grouping problem is now fixed, would it make sense to add corresponding testcases? They are already added by http://gcc.gnu.org/ml/gcc-cvs/2013-07/msg00643.html (though I found the changelog entry used old file names, I'll fix it later). This time it's the BFS approach that can correctly handle the problem instead of the DFS one. I think, for now, they can share the testcases. -- Tim Shen
Re: [Patch] Refractor Thompson matcher
Hi, On 07/28/2013 12:18 PM, Tim Shen wrote: They are already added by http://gcc.gnu.org/ml/gcc-cvs/2013-07/msg00643.html (though I found the changelog entry used old file names, I'll fix it later). This time it's the BFS approach that can correctly handle the problem instead of the DFS one. I think, for now, they can share the testcases. I see. I was wondering if in this development stage it would be convenient to have somewhere a parameter allowing to switch by hand such internal details, useful for testing purposes too. Eventually may or may not go away. Paolo.
Re: [Patch] Refractor Thompson matcher
On Sun, Jul 28, 2013 at 9:44 PM, Paolo Carlini paolo.carl...@oracle.com wrote: I see. I was wondering if in this development stage it would be convenient to have somewhere a parameter allowing to switch by hand such internal details, useful for testing purposes too. Eventually may or may not go away. Here it is :) Github : https://github.com/innocentim/gcc/tree/tim/regex/libstdc++-v3/include/bits -- Tim Shen bfs.patch Description: Binary data
Re: [Patch] Refractor Thompson matcher
On 07/28/2013 05:50 PM, Tim Shen wrote: On Sun, Jul 28, 2013 at 9:44 PM, Paolo Carlini paolo.carl...@oracle.com wrote: I see. I was wondering if in this development stage it would be convenient to have somewhere a parameter allowing to switch by hand such internal details, useful for testing purposes too. Eventually may or may not go away. Here it is :) Oh well, thanks. But then I expect specific testcases to come with it, for the various values of the parameter, both the default and the other other values. Otherwise, the idea isn't really immediately useful. See what I mean? Also, please make sure that this change is in any case a step in the right direction. I mean: I suppose that if we do this only the logic to control automatically the switch is missing and when we add it we are done. Let's not do it if I'm mistaken. Thanks! Paolo.
Re: [Patch] Refractor Thompson matcher
On Mon, Jul 29, 2013 at 1:08 AM, Paolo Carlini paolo.carl...@oracle.com wrote: Oh well, thanks. But then I expect specific testcases to come with it, for the various values of the parameter, both the default and the other other values. Otherwise, the idea isn't really immediately useful. See what I mean? So I modified testcases to test both of them. Also, please make sure that this change is in any case a step in the right direction. I mean: I suppose that if we do this only the logic to control automatically the switch is missing and when we add it we are done. Let's not do it if I'm mistaken. Never mind, this patch makes them switch to be automatic(based on whether there're back-references in the regex), thanks to the full-featured regex scanner. A testcase is given too. -- Tim Shen dispatch.patch Description: Binary data
