Quick fix for the duplicate ID issue

[vitaut]

This change suggested by @dean0x7d fixes SEVERE: Duplicate ID warnings and missing anchors in case of overloads. Here's an example that reproduces the problem (requires Sphinx version sphinx-doc/sphinx@1021f4c)

/** Test */
char operator"" _format(const char *s, std::size_t);

wchar_t operator"" _format(const wchar_t *s, std::size_t);

but I guess the same may apply to other overloads.

Here are some more details: fmtlib/fmt#207 (comment).

I'm not sure what's the best solution here. @michaeljones, do you think options should be passed to renderer_factory_creator_constructor.create_factory_creator at all?

[michaeljones]

Thanks for tracking all this down and debugging it. Sorry it has happened. Especially as I've seen the SEVERE warnings before and not taken the time to investigate. I'll check this out now.

[michaeljones]

Ok, I've pushed a branch called dean0x7d-overloads_bug which is largely the same logic but moved around a little bit. I think it feels a little better here but I might just be pushing code around to make myself feel like I'm contributing :)

I'm pretty sure it should work but I'm afraid I don't have the full test set up here with your code and the appropriate version of Sphinx.

The thousands of factories do make me smile a bit now. There is method behind the madness but it does look a bit mad. I don't have the will power to try to refactor it :)

[vitaut]

I've tested dean0x7d-overloads_bug branch and confirm that it works fine on my example. Looking forward to see it merged in the master.

Thanks for refactoring. I was thinking about getting rid of passing options myself but was not sure if it was the right thing to do =).

[michaeljones]

Thanks for checking it over so quickly. Merged into master. Shout if you want a release.

[rweickelt]

Hey guys. You're doing a great job! Thank You very much for all the efforts.

The thousands of factories do make me smile a bit now. There is method behind the madness but it does look a bit mad. I don't have the will power to try to refactor it :)

Michael, it feels like there are zillions. I've had a hard time to put the puzzle together, but finally gave up after some hours :-/ It's really difficult to read. I'm not moaning at all, just being relived that I'm not the only person with that opinion.

[michaeljones]

I'm going to ramble a bit in an opinionated fashion...

This is might be obvious or crazy but the theory is that if you want to create a new object and you need state to create it, normally some of that state is available early on (state_a) and some of it is available only when you need to create the object (state_b). So if take the state_a that is available early on, wrap it in a factory and have it create the object later on using a create method that takes state_b as an argument then you only need to pass that factory (rather than all the bits of state_a) around and you also have some control over dependencies if you need to mix things up a bit. (ie. you can replace that factory with one that makes a slightly different object and the rest of the code is none the wiser.)

Breathe's code base is kind of just a relentless application of this rule. Unfortunately, sometimes you have state_a, state_b and state_c which are all available at different times so you have a FactoryFactory, a Factory and a final object. Sadly it seems that I managed to make it to FactoryFactoryFactory at some point.

The architecture keeps things fairly flexible. Some changes end up being super easy because everything is broken down into these hierarchies. When you're working with the code a lot then the pattern & the rough picture stays in your head and it is pretty manageable. When you step away for 6 months or you're new to the code base then it is pretty painful. I've always felt this with code. Either you optimise for the maintainer or for the new comer. For a new comer you tend to have fewer objects and more large methods with if-statements where the various pathways can be discovered by staring at one or two methods for long enough. For the maintainer, you end up with lots of small objects/functions and more dependency injection and interfaces to reduce code duplication which makes it easier to change but with the cost of increasing complexity as far as understanding it.

Another way of looking at is local readability vs global readability. A mass of small objects is more readable if you know what they all do. It becomes a higher level vocabulary in which you are writing your code but if you're new you often cannot understand any one 10 line function without reading a half dozen more and tracing all the injected dependencies. More locally readable code tends to sacrifice global readability as you don't have that higher level vocabulary any more. Everything is spelt out for you to understand but in basic words so long functions are required to achieve complex things, flexibility is achieved with flags & if-statements and the control flow needs to be carefully followed to understand exactly what is going on.

The other aspect is that Breathe is now 6 years old and covers a large chunk of my programming career which means that some of it reflects a well meaning but inexperienced coder. That said, I do believe in this maintainer vs newcomer trade off and whilst I find the factories bewildering at times I feel that each largely has its place though perhaps a more functional style with some factories replaced with closures which might dodge the issue of naming stuff with an increasing number of 'Factory' suffixes :)

It is certainly possible that as I gain more experience that I will see a better way of managing it but for the moment it remains a conscious choice. Perhaps a poor one for an open source project that would benefit from more contributors but still a choice that I would find hard to revert as it would mean deliberately stepping in and making the code less flexible and maintainable for the sake of readability.

If it helps, I've had trouble with this at work from time to time. Some people clearly prefer the locally readable approach. Having played with both I favour the global style but perhaps that is more a function of how my brain works rather than some kind of universal objective truth about which is better.

If any of this seems reasonable or rings bells and you can recommend some reading materials for perhaps getting to the next step along the programmers journey, I'd be very happy to have some advice.

Ramble over. Thanks for indulging me. It is fun to brain dump from time to time.

[ishitatsuyuki]

Seems this is happening again after refactoring. Can you guys confirm? Maybe open a new issue?

[vitaut]

I don't get Duplicate ID issue, but there is a different warning (fmtlib/fmt#252):

/home/viz/workspace/sphinx-test/index.rst:1: WARNING: doxygenfunction: Unable to resolve multiple matches for function "operator""_format" with arguments (const char *s, std::size_t) in doxygen xml output for project "sphinx-test" from directory: doxyxml.
Potential matches:
    - char operator""_format(const char *, std::size_t)
    - wchar_t operator""_format(const wchar_t *, std::size_t)

@ishitatsuyuki are you referring to it?

[ishitatsuyuki]

Seems this approach is too hacky.

My situation is a bit different, which has the following function:

const T get() const;
T get();

This is still a valid overload in C++.

You can check here for the full build log:
https://gitlab.com/itxtech/cenisys/builds/942003#down-build-trace

[vitaut]

I've opened a separate issue: #246

[michaeljones]

For some reason I find myself wanting to document that I eventually came around a bit and started trying to make the code more understandable with the kind help of others:

#236

I'll now stop reading old issues and try to help with the code a little.