First batch of merges from the backlog.

[anadon]

Added PR's #80, #93, #95, #111 These are well formed bugfix PR's which should be added. All tests are passing locally, but I don't have the facilities to test more broadly. The broader test facilities would be a future feature addition and out of scope of what I've been immediately tasked with.

[jeremy-murphy]

@anadon I just want to say thank you so much for getting things moving! I have recently been trying to help some contributors get their PR into good shape to smooth the acceptance process, but I didn't expect that someone with merge permission would swoop in so soon to actually clear up the backlog. Cheers.

[anadon]

I don't have write permissions. I'm just making things easier.

[jeremy-murphy]

Oh, OK. Why are you merging multiple PRs into batches? If they get merged as a batch and we realize afterwards that something broke, won't it be harder to identify and revert the specific original PR that caused the break?

[anadon]

I was directed to start catching up on the backlog and this was more expedient. On balance, it is better to get the obvious and easy stuff in quick since there are PR's waiting since 2014 and then suss through the ones that aren't trivial.

If I end up the active maintainer for graph, or someone else dedicates time, then pull requests could be handled as they come which is more proper.

[jzmaddock]

I was directed to start catching up on the backlog and this was more expedient.

Actually I meant you to batch the trivial one AFTER CI was set up, otherwise it's just a bunch of changes mashed together ;) Probably I didn't make that clear - sorry about that!

Anyhow I've enabled travis and appveyor, time permitting I'll sort out some CI scripts later...

[jzmaddock]

OK CI is now enabled, can you "touch" one of the files in this PR? Hopefully that will trigger a fresh CI build.

There is currently one CI failure in develop: https://travis-ci.org/boostorg/graph/jobs/423317402

relaxed_heap_test fails for gcc-8.1 in C++03 mode, I can reproduce here and gdb reports:

0%   10   20   30   40   50   60   70   80   90   100%
|----|----|----|----|----|----|----|----|----|----|
*Assertion failed!

Program: D:\data\boost\boost\bin.v2\libs\graph\test\relaxed_heap_test.test\gcc-8.1.0s03\debug\relaxed_heap_test.exe
File: ..\..\../boost/optional/optional.hpp, Line 1205

Expression: this->is_initialized()

I've no idea if this is a graph or Boost.Optional issue.

[anadon]

Let me sort through this later today. I'm just waking up after a late night.

[anadon]

Looks like relaxed head is being used as a priority heap, mainly coded in 2004. I think replacing it with boost.heap would be a good idea. Thoughts @jzmaddock ?

It also looks like there are a number of 'pending' files which need attention from way back when.

[anadon]

Actually, I'm looking at the stdlib implementation of priority queues and heaps and they are old enough that they look like they might be drop in replacements. I'll look into how viable those are. There is a lot of complexity in graph that I'm hoping is not necessary.

[anadon]

It actually looks like the relaxed_heap is not used elsewhere, users don't have a method to enable it, a google search yields no used examples for a total of 6 results, and related structures in code are just not used. With it being vestigial, I'd like to remove it. I'd like a go-ahead before I muck around with that.

[jzmaddock]

It actually looks like the relaxed_heap is not used elsewhere, users don't have a method to enable it, a google search yields no used examples for a total of 6 results, and related structures in code are just not used. With it being vestigial, I'd like to remove it. I'd like a go-ahead before I muck around with that.

A quick bit of grepping around: There are a few headers which include it, but don't use it, those #includes should be removed.

It is still used in dijkstra_shortest_Paths.hpp if BOOST_GRAPH_DIJKSTRA_USE_RELAXED_HEAP is defined, but this is not the default, and it appears not to be documented anywhere.

So... my suggestion would be to remove all references to it, but leave the header in place, and add some #pragmas to it to loudly report it as deprecated and unsupported.

[anadon]

There are things in iteration_macros.hpp that I never could have imagined are valid syntax. They certainly are not in best practice. Attending to those now. I don't know why my system is not picking these up.

[swatanabe]

#80 looks suspect to me. It claims to be a
straight up bug fix, but the actual code change
is more than that. The original code forbids adding
a vertex that is already present in the subgraph
and this appears to be intentional, because of the
assert. Changing the preconditions like this, at
a minimum, requires clarification in the documentation
about exactly what the preconditions are.

[anadon]

@swatanabe Could you be more specific in the issue you are seeing?

[swatanabe]

AMDG

On 09/01/2018 05:45 PM, jrmarsha wrote: @swatanabe Could you be more specific in the issue you are seeing?

The PR claims that it fixes a bug wherein add_vertex fails to add the vertex to the ancestors of a subgraph, breaking the subgraph invariants. That's all well and good, so far, and the new behavior is clearly correct according to the documentation. The problem is that it /also/ permits inserting a vertex into the root and inserting a vertex which is already present, both of which were previously disallowed. It is not clear from the documentation whether these should be allowed or not. I am not claiming that these additional changes are necessarily wrong, but API changes like this need to be done intentionally, and not as an inadvertent side-effect of other changes. In Christ, Steven Watanabe

[anadon]

I'll review it when I get the current builds to stop failing. Thank you for bringing it to my attention.

[pdimov]

No more undermining will be forthcoming.

John, I fixed the g++ 4.6 issue with Boost.Range, and the results here will show up at https://travis-ci.org/boostorg/graph/builds/435122722, when Travis gets around to it. The branch is feature/gcc-4.6, I leave the decision to merge (if the tests pass) to you.

In addition, I've added an -fsanitize=undefined job on feature/ubsan, results should show up at https://travis-ci.org/boostorg/graph/builds/435123731.

So... my suggestion would be to remove all references to it, but leave the header in place, and add some #pragmas to it to loudly report it as deprecated and unsupported.

Since dijkstra_shortest_paths.hpp unconditionally includes relaxed_heap.hpp, the loud reports show up on each direct or indirect use of it. The include should probably be either guarded by #ifdef BOOST_GRAPH_DIJKSTRA_USE_RELAXED_HEAP, or removed outright (along with the corresponding code block.)

[jzmaddock]

John, I fixed the g++ 4.6 issue with Boost.Range, and the results here will show up at https://travis-ci.org/boostorg/graph/builds/435122722, when Travis gets around to it. The branch is feature/gcc-4.6, I leave the decision to merge (if the tests pass) to you.

Merged, thanks.

In addition, I've added an -fsanitize=undefined job on feature/ubsan, results should show up at https://travis-ci.org/boostorg/graph/builds/435123731.

Ugh, lots of failures - looks like all the same failure and that the library plays fast and loose with some typecasts?

Since dijkstra_shortest_paths.hpp unconditionally includes relaxed_heap.hpp, the loud reports show up on each direct or indirect use of it. The include should probably be either guarded by #ifdef BOOST_GRAPH_DIJKSTRA_USE_RELAXED_HEAP, or removed outright (along with the corresponding code block.)

Good catch - I would tend to vote for a #if guard as the most conservative option, but don't have a strong view.

The way you're going about this is counter productive. Please don't undermine this PR.

I'm sure that's not the intention at all. I realise this is all getting way more complex than it first appeared, but unfortunately when a library isn't maintained for a while it can take a while to bash things into a shape where substantive patches can even be considered. Once everyone is confident the testing regime is sound, then patches like this become much easier to evaluate.

That just leaves the change in semantics in #80 highlighted by @swatanabe - let me see if I can produce a test case so we can all decide what the right thing to do is.

[jzmaddock]

OK, I'm going to write out what I think is happening here and you guys can all tell me I'm wrong :)

• You cannot add a vertex to the root graph - the vertexes are already there when you create the graph, you can only join them together with edges. Hence the original code had BOOST_ASSERT(!g.is_root());
• The PR (correctly IMO) recursively adds vertexes to parent subgraphs so that we maintain the invariant that each child is a strict subset of it's parents.
• The PR uses is_root() to determine when to abort from the recursion - the effect is to remove the assert and make adding vertexes to the root a no-op instead.
• I'm in 2 minds whether the assertion is important to retain: if we wished to reinstate it we could just change add_vertex(u_global, g.parent()); to if(!g.parent().is_root()) add_vertex(u_global, g.parent());.
• The only use case I could think of where the assertion might be important, is where an attempt is made to add a vertex with an invalid index - but I see this isn't checked for when adding vertexes to sub-graphs either, so perhaps it doesn't matter?

[pdimov]

Ugh, lots of failures - looks like all the same failure and that the library plays fast and loose with some typecasts?

False positives caused by the hidden visibility. I'll try to patch these up.

I'm trying to convince the visibility experts that visibility should propagate, so that we can just say b2 visibility=global on these runs and not have to specify it everywhere manually (boostorg/build#333) - no luck so far.

[pdimov]

The integer overflow in floyd_warshall_test is not a false positive caused by hidden visibility though. :-)

[anadon]

Can some of these easier ones be left to me? I'm trying to learn more about this.

…

On Sun, Sep 30, 2018, 09:00 Peter Dimov ***@***.***> wrote: The integer overflow in floyd_warshall_test is not a false positive caused by hidden visibility though. :-) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#114 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC-CBSvmKdmnvsgnm_U4dEYWCZKnQxxwks5ugMBVgaJpZM4WWD51> .

[anadon]

@jzmaddock Thinking on the subgraph stuff more, I think it tends to work like sets and subsets. So a set always contains the items in it's subsets, but the converse is not true. So then, you can add items to a subset and transitively in the larger set. New vertexes can be added to the graph but are not added to a subgraph by such an insertion, but adding a new vertex to a subgraph adds it to the graph. This is the only variation from what you're saying. I'm not confident what the implementation intends, though.

@pdimov What about adding other sanitizers? Could add the following: -fvtable-verify=preinit -fstack-check -fstack-protector-all -fsanitize-address-use-after-scope -fsanitize=leak -fsanitize=address? The bugs -fsanitize=undefined seem easy enough to fix and the extra sanitizers should also catch similar difficulty bugs I can work on.

[pdimov]

Graph would benefit from -fsanitize=address, but I have to get undefined working first, which is taking more than expected. Not familiar with the rest.

[anadon]

@pdimov What are you running into with undefined?

[pdimov]

There were false positives caused by -fvisibility=hidden, which I needed to work around, and a real issue in Boost.Serialization, for which I had to disable the serialize.cpp under UBsan. The results are at https://travis-ci.org/boostorg/graph/builds/435276340, the only remaining failure is a legitimate integer overflow.

[jzmaddock]

Thinking on the subgraph stuff more, I think it tends to work like sets and subsets. So a set always contains the items in it's subsets, but the converse is not true. So then, you can add items to a subset and transitively in the larger set. New vertexes can be added to the graph but are not added to a subgraph by such an insertion, but adding a new vertex to a subgraph adds it to the graph. This is the only variation from what you're saying. I'm not confident what the implementation intends, though.

Yes that's pretty much it, but we have 2 overloads of add_vertex:

vertex_descriptor
add_vertex(subgraph& g);

Adds a new vertex to the subgraph and all of it's parents, and then the one that we're changing:

vertex_descriptor
add_vertex(vertex_descriptor u_global, subgraph& g);

Takes an existing vertex from the root graph, and adds it to subgraph g. Logically it makes no sense to call this for the root graph because it's a precondition that u_global is in that graph already.

[pdimov]

Live results with -fsanitize=address,undefined at https://travis-ci.org/boostorg/graph/jobs/435291924.

I had to disable LeakSanitizer because it kept encountering a fatal error: https://travis-ci.org/boostorg/graph/jobs/435286728.

[swatanabe]

AMDG

On 09/30/2018 10:15 AM, jzmaddock wrote: <snip> ``` vertex_descriptor add_vertex(vertex_descriptor u_global, subgraph& g); ``` Takes an existing vertex from the root graph, and adds it to subgraph `g`. Logically it makes no sense to call this for the root graph because it's a precondition that `u_global` is in that graph already.

Another way of expressing this is: - Pre: The vertex must be present in the root graph. - Post: The vertex is present in the subgraph. vs. - Pre: The vertex must be present in the root graph, but not in g. - Post: The vertex was added to the subgraph The original code seems to follow the latter, while the new code obeys the former. I'm fine with this change, provided /the documentation is clarified./ Simpler is better. In Christ, Steven Watanabe

[jeremy-murphy]

I don't understand why this PR groups several other PRs together. The original PRs should be merged one by one, as needed.

Although I appreciate anyone making an effort, I agree with Peter and really discourage this approach to merging multiple PRs in one go. The primary issue is the conflation of fixes into a single merge, which doesn't allow for reasoning, or changing our mind, about each change independently. The secondary issue is that it might seem expedient to the person that can push to this branch, but, what about all the contributors that opened the original PRs? As far as I could tell, the real blocker to progress was the absence of a maintainer to review and merge the extant PRs, not the absence of one more PR.
As I said, I really appreciate anyone making an effort, but I struggle to support this way of tackling the backlog.
No offence meant to anyone, but my sincere suggestion is to close this PR and continue the work on each respective PR independently. There seems to be momentum for it now, and I assume, @jzmaddock , that you have merge permission?

[jzmaddock]

Although I appreciate anyone making an effort, I agree with Peter and really discourage this approach to merging multiple PRs in one go. The primary issue is the conflation of fixes into a single merge, which doesn't allow for reasoning, or changing our mind, about each change independently. The secondary issue is that it might seem expedient to the person that can push to this branch, but, what about all the contributors that opened the original PRs? As far as I could tell, the real blocker to progress was the absence of a maintainer to review and merge the extant PRs, not the absence of one more PR.

That's my fault/suggestion really - the issue is that the library had no CI set up and working - I've fixed that now, but I believe there is no way we can trigger CI runs on old PR's (that pre-date the CI additions) so there's no benchmark for us to judge them by. There's also no way for us to push to these branches are they're owned by the original submitter.... we can ask for the original submitter to make changes, but if they're trivial it's often easier to just start a new branch and PR for integration testing.

Please prove me wrong on the above - nothing would delight me more ;)

There seems to be momentum for it now, and I assume, @jzmaddock , that you have merge permission?

Yup, and I think this one is about ready - I just need a bit of time to double check.

[anadon]

So do I just close this, then?

[mohe2015]

@jzmaddock

the issue is that the library had no CI set up and working - I've fixed that now, but I believe there is no way we can trigger CI runs on old PR's

Try to close and reopen the PRs. That MAY trigger a CI run.

[pdimov]

It will, but it's better to ask their authors to rebase against the current develop.

[jzmaddock]

It will, but it's better to ask their authors to rebase against the current develop.

Isn't that unnecessary unless there is a merge conflict (in which case the CI tests won't be run anyway)?

[pdimov]

I think that pull requests that have been created at a time there was no CI will not be CI-tested, no matter how many times we reopen them. :-)

[pdimov]

But I could be wrong about this.

[jzmaddock]

But I could be wrong about this.

You are - it worked on the one I just tried :)

[jzmaddock]

Merged to develop.

I've also re-instated the removed assert as per @swatanabe comment.

[anadon]

@jzmaddock Holy inbox batman! I will ping you for small stuff from now on. Suddenly all the graph.

[jeremy-murphy]

I hope this works out as much as anyone, and I thank @anadon for their effort, but if something is mysteriously wrong and requires reversion of this merge and going back to square one, I will say "I told you so". :)

[anadon]

@jeremy-murphy I know...