forward declarations should be removed

[Laeeth]

if the .d file declares struct Calendar{ Date date; } and before that there is struct Calendar; then the forward declaration struct Calendar; should not end up in the .d file

[Laeeth]

fixed https://github.com/kaleidicforks/dpp/commit/234f99546f290b8ffec47c42ea0e5b7c9db81cd1 and a bug fixed here: https://github.com/kaleidicforks/dpp/commit/5a3b96a8094f0d0b93b10e56fb99295e156281b6

we skip forward declarations for aggregates and only want forward declarations for functions/methods

[Laeeth]

spoke to soon - a bug. bug is fixed - see above.

[atilaneves]

we skip forward declarations for aggregates and only want forward declarations for functions/methods

This isn't true - I expect that if you run the tests on your fork they will fail. This code in particular from this test:

struct Struct;
void fun(struct Struct* s);
struct Struct* make_struct(void);

Forward declarations are sometimes necessary. I can't conceive of a way dpp would work on any real life codebase currently if this wasn't currently being handled.

I'm having trouble understanding what the issue is. The C code below correctly gets translated to the D code below it in turn:

struct Foo;
struct Foo { int i; };

struct Foo
{
    int i;
}

if the .d file declares struct Calendar{ Date date; }

Why is the D file be manually declaring C/C++ types?

[Laeeth]

It's an error if dpp produces a forward declaration in the D file and then goes ahead and defines the aggregate later on in the same file. It's also an error if a method is declared in an aggregate and subsequently declared in the same file outside of the aggregate (coming from the definition). It's double plus an error if the subsequent declaration is outside the aggregate and missing a this pointer.

I don't doubt that it works for C, because it's now rare to find a C header that it won't parse. But for even quite simple C++ it fails, possibly due to namespace interactions. And I can't check your test right now, but I think you're right that my fix is slightly off.

I should have just for aggregates excluded the aggregate if the definition of the cursor is not null and the cursor is not identical with the definition of the cursor.

Who said anything about manual? It's the output from dpp that's at fault. If you would try running it over Xenon as I suggested you will see quite quickly.

Do you agree that the cases I describe are pathological? Since the file won't compile I don't see how their can be a debate...

I think maybe it's namespace related and how you remember structs. But why do you need to remember them when clang already knows whats what? I am tired and don't recall the code so that might not be it.

[Laeeth]

Given C++ allows you to define methods outside aggregates then I believe either you will have to adopt my fix or you will need to filter them out based on getCursorSemanticParent. That's why the problem occurs at all. For functions/methods as far as I can see only declarations should appear in the .D file. The definitions should be stripped.

[Laeeth]

I remember now. I decided to do what I did and kill aggregate forward definitions because you manually go back and add missing ones anyway.

[atilaneves]

Do you agree that the cases I describe are pathological? Since the file won't compile I don't see how their can be a debate...

I agree there's no debate if dpp can't handle a valid C++ file. I'll have to try running it on xenon myself.

[atilaneves]

This issue was in all likelihood caused by how namespaces used to be handled. Now that they're no longer reopened and all cursors are aggregate across namespace declarations with the same name, I doubt this exists. Since there's no concrete example to check against and I haven't run into this in the xenon code I've been running d++ on, I'll close it.

[Laeeth]

But it's legal in C to say struct Foo; struct Foo; ad nauseum. More usually via headers and macros than explicitly. Since it's legal in C and illegal in D and since DPP is designed to just work unlike dstep, and since no harm can come of removing forward declarations ahead of the definition itself, what harm can come of this?

[atilaneves]

But it's legal in C to say struct Foo; struct Foo; ad nauseum**

Yes.

what harm can come of this?

I think I wasn't clear - dpp has filtered clang cursors so that there is only ever one declaration since before v0.0.1 - how it would work otherwise on Python, Postgresql, or any other real project?

I'm not saying "dpp shouldn't deal with forward declarations" - I'm saying "dpp already does this". The difference is that when I introduced handling namespaces for C++, the filtering was done differently and ended up having bugs. But only for C++, only for namespaces, and only in certain ways.

Without an actual example in this issue, it was hard for me to understand what was happening until I ran dpp on xenon myself, and have since fixed it in a better way.

[Laeeth]

Yes, okay. I said the same originally - it's to deal with a different problem and then I forgot myself. You're right.