New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make var & function declaration/definition location parsing not order-dependent #6722

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@mosra
Copy link
Contributor

mosra commented Dec 31, 2018

The m.css Doxygen theme provides include information also for free functions, variables and typedefs (instead of just classes), as can be seen for example here:

image

To do that, the script is extracting the <location> information from the XML files. Previously, if a definition was processed before declaration, both the declaration and definition location was set to the definition. It was working correctly only when declaration was found before definition, but due to the fact that C/C++ files are named like this:

foo.cpp
foo.h  

And thus when globbing the filesystem, the definition is always seen before the declaration. Note that this affects only functions inside namespaces, (in my testing) functions in global scope were reported
twice, once for each file (which can be considered okay, as it doesn't exhibit this bug).

For variables, I'm distinguishing definitions and declarations based on presence of the extern keyword -- declarations have it, definitions not.

In case it's not clear what this PR does -- there's a test in the first commit, run it with current Doxygen master to see the difference. Oh, also -- I'm not well aware of all the interactions in Doxygen internals, so hopefully changing the source location this way doesn't lead to any inconsistencies in the internal state.

mosra added some commits Dec 31, 2018

Test proper parsing of var & function declaration / definition location.
Fails. Longer explanation and fix in the next commit.
Make var & function declaration/definition location not order-dependent.
Previously, if a definition was processed before declaration, both the
declaration and definition location was set to the definition. It was
working correctly only when declaration was found before definition, but
due to the fact that C/C++ files are named like this:

    foo.cpp
    foo.h

And thus whe globbing the filesystem, the definition is always seen
before the declaration. Note that this affects only functions inside
namespaces, (in my testing) functions in global scope were reported
twice, once for each file (which can be considered okay, as it doesn't
exhibit this bug).

For variables, I'm distinguishing definitions and declarations based on
presence of the extern keyword -- declarations have it, definitions not.

Note that this is still a bit random when there's more than one
var/function declaration, but the new behavior is still better than
conflating the definition/declaration locations into one. Also it's
very probable that only one declaration will have a corresponding
Doxygen doc block and others can be hidden from Doxygen using a
preprocessor.
Show resolved Hide resolved src/doxygen.cpp
@doxygen

This comment has been minimized.

Copy link
Owner

doxygen commented Jan 10, 2019

@mosra My idea was to always point to the definition and only fall back to the declaration if no definition is available (e.g. in case one only feeds the header file to doxygen). If I change it this way it is also order independent, but I don't know if you need the location of the declaration as well?

@mosra

This comment has been minimized.

Copy link
Contributor

mosra commented Jan 10, 2019

That's not how the parsing works at the moment, though (from my experience, at least). TL;DR taken from the test file -- if I have this (with line numbers):

1: // a.cpp
2:
3: void foo();
4: 
5: void foo() {}

then Doxygen produces this in the XML file (which makes sense, and nope, no fallback to the declaration is happening):

<location file="a.cpp" line="3" column="1" 
          bodyfile="a.cpp" bodystart="5" bodyend="5"/>

However, in the opposite case, when a declaration is found after a definition:

1: // b.cpp
2: 
3: void foo() {}
4: 
5: void foo();

then, with current master, I have both definition and declaration pointing to the same place:

<location file="b.cpp" line="3" column="1" 
          bodyfile="b.cpp" bodystart="3" bodyend="3"/>

Since declarations and definitions are usually in separate files, this depends on the order in which the file are discovered and that depends on many external factors (order in INPUT, extensions used etc.). Thus, in this patch, I changed the second case to produce this instead:

-<location file="b.cpp" line="3" column="1" 
+<location file="b.cpp" line="5" column="1" 
           bodyfile="b.cpp" bodystart="3" bodyend="3"/>

The reason why I want this is that now file/line/column always points to the declaration (if found), and the declaration is generally in the header the user is meant to #include -- which I then use to show correct #include information for free functions.

In case one feeds only header files to doxygen, no pairs of declarations + definitions are found, thus this conflict-resolving code get never executed. And so the behavior in such case does not change with my patch.

Hope this finally clears my intentions up a bit :)

@doxygen

This comment has been minimized.

Copy link
Owner

doxygen commented Jan 12, 2019

I've push the following commit 7f40e48.

It adds new declfile, declline, and declcolumn attributes to location in order to store information about the declaration (if found), e.g.:

    <location file="082_decl_def.cpp" line="11" column="1" 
      bodyfile="082_decl_def.cpp" bodystart="11" bodyend="-1" 
      declfile="decl_def.h" declline="5" declcolumn="1"/>

As mentioned before, the file, line, and column attributes are meant to store definition information if found and fall back to the declaration info if not. This should now be done independent of the order. I've added test cases 082 and 083 for testing both orders.

So there are now 3 situations:

  • doxygen finds a declaration and a definition for a variable or function: in this case file,line,column always points to the definition, and declfile,declline,declcolumn points to the declaration
  • doxygen only finds a declaration for a variable or function: in this case both file,line,column and declfile,declline,declcolumn point to the declaration.
  • doxygen only finds a definition for a variable or function: in this case file,line,column points to the definition and the declaration attributes are omitted.
@mosra

This comment has been minimized.

Copy link
Contributor

mosra commented Jan 12, 2019

Oh, that's wonderful, thank you! 👍 Just tested and it seems to be doing the right thing for all cases I had -- so first I check if declfile is there and then if not, I fall back to file.

What's the desired use of bodyfile, by the way? My original assumption was that this is the definition location (which caused all the confusion above), but that's apparently file?

I guess this PR can be closed then, right? :)

@doxygen

This comment has been minimized.

Copy link
Owner

doxygen commented Jan 13, 2019

bodyfile belongs together with bodystart and bodyend to indicate where the implementation body of a function is. I agree that bodyfile itself is rather redundant (it is always the same as file now, but it is kept for backwards compatibility.

@doxygen doxygen closed this Jan 13, 2019

@mosra mosra deleted the mosra:fix-order-dependent-declaration-location branch Jan 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment