Skip to content
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

Fix script errors reporting wrong line numbers #937

Merged
merged 34 commits into from
Jun 30, 2018

Conversation

dedmen
Copy link
Contributor

@dedmen dedmen commented Jun 11, 2018

preprocessFileLineNumbers "x\cba\addons\common\fnc_currentUnit.sqf"
Before: https://gist.github.com/dedmen/896504ced548b9e2f9de738525908510
After: https://gist.github.com/dedmen/1ffc539dbec1d016d602b62888f94943

Let's superimpose the actual script together with the line numbers.

Before:
firefox_2018-06-11_21-17-13

Was the getVariable on line 6? Is that correct? NO!

missionNamespace getVariable ["bis_fnc_moduleRemoteControl_unit", player]

That's line 19!

After:
firefox_2018-06-11_21-15-43
Was the getVariable on line 19? Is that correct? YES!

missionNamespace getVariable ["bis_fnc_moduleRemoteControl_unit", player]

That's indeed line 19!

The above will cause script error messages to be on the wrong line exactly like I showed you here. Because the in-engine representation is broken because of this preprocessor bug.
And because the game doesn't know where the F it is right now, neither does my debugger. Which makes CBA not debugable. You set a breakpoint on Line 19, but never hit it because Line 19 is actually Line 6.

This PR is WIP. I will add all the other files too. Just wanted to get this visualization out before I actually get everything done.

Scripts that grab the header to generate documentation probably also have to be updated.

@ViperMaul
Copy link
Contributor

ViperMaul commented Jun 12, 2018

Nice @dedmen
Also what we need is a regex script to apply your same fix to all of the other SQF files that have this issue. There are well over 300+ additional files where #include "script_component.hpp" is located below comments.

Looks like you are already on that path. Thank you!

@kymckay
Copy link
Contributor

kymckay commented Jun 12, 2018

This has bugged me for so long in ace too, good solve 👍

@kymckay
Copy link
Contributor

kymckay commented Jun 12, 2018

For the regex this should do it:

^\s*(/\*.+?\*/)\s*#include\s*"script_component\.hpp"

In sublime add the flags (?s)(?-m) to the front to disable multiline and enable dotall.

Replace with:

#include "script_component.hpp"\n\1

@dedmen
Copy link
Contributor Author

dedmen commented Jun 12, 2018

Looks like you are already on that path. Thank you!

correct I wanted to... Oh.. @SilentSpike Don't steal my work!!!!!111!11"
I'll also do the ACE PR when I find the time. Expect atleast a couple weeks
My main priority is Debugger support. And the Debugger plugin will still take a bit-ish.

@dedmen
Copy link
Contributor Author

dedmen commented Jun 23, 2018

Does anyone know if the documentation generator tool thingy can handle this without problems?
Could already use this in new PR's like https://github.com/CBATeam/CBA_A3/pull/939/files#diff-57a7e296bee76251cea2aa15e625db0bR25 before this is ready so that I don't have to change new things.
I'll try to think about this task tomorrow. Maybe I can get it all done in a day

@@ -24,8 +25,6 @@ Examples:
Author:
Written by Deadfast and made CBA compliant by Vigilante
---------------------------------------------------------------------------- */

#include "script_component.hpp"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't match the default style. So I'd say it's okey that, that newline is gone?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, not sure if you used my regex, but I specifically made it account for cases where there might be space and strip it out

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used yours. But slightly modified [\s\S] instead of .

@dedmen
Copy link
Contributor Author

dedmen commented Jun 24, 2018

What about files that don't have the include? Like https://github.com/CBATeam/CBA_A3/blob/master/addons/ai/fnc_searchNearby.sqf

lol wtf

#include <script_component.hpp>

What should I do with this:

#define DEBUG_MODE_NORMAL
#include "script_component.hpp"

#define DEBUG_MODE_NORMAL
#include "script_component.hpp"

scriptName "fnc_polar2vect.sqf";
#include "script_component.hpp"
SCRIPT(polar2vect);

Shouldn't the SCRIPT macro take care of scriptName?

#define DEBUG_SYNCHRONOUS

?

@dedmen
Copy link
Contributor Author

dedmen commented Jun 24, 2018

The last commit has stuff that was not just done automatically. Take special care about that when reviewing the PR.

I'm done now. Just need someone to check the makeDocs script with this. I don't have perl or "NaturalDocs"

@commy2
Copy link
Contributor

commy2 commented Jun 24, 2018

Merge asap, to avoid merge conflicts.

@dedmen
Copy link
Contributor Author

dedmen commented Jun 24, 2018

I'll happily fix conflicts. If you merge #939 I'll have to add these anyway.
I'd rather wait for atleast 2 reviews to make sure this is right. Also there are open questions from me above and the documenation generator needs to be tested.

If something is wrong in here it could break CBA at any point, sensitive or not. Don't wanna take that risk

@kymckay
Copy link
Contributor

kymckay commented Jun 24, 2018

The last commit looks all good 👍

@dedmen
Copy link
Contributor Author

dedmen commented Jun 25, 2018

Just waiting for #937 (comment)
and #937 (comment)
🕐 ⏳ ⏲ 🌳 🍪

@commy2
Copy link
Contributor

commy2 commented Jun 25, 2018

Patience. Will review this week. Promised.

@dedmen
Copy link
Contributor Author

dedmen commented Jun 25, 2018

You wanted it merged asap, not me :D
merge conflicts shouldn't be an issue as this only changes essentially the first line of the scripts. Which is usually never touched.

@commy2 commy2 added this to the 3.8 milestone Jun 25, 2018
@commy2 commy2 added the Bug Fix label Jun 25, 2018
@commy2 commy2 self-assigned this Jun 25, 2018
@commy2 commy2 merged commit fbd59d2 into CBATeam:master Jun 30, 2018
@jonpas jonpas changed the title [WIP] Fix script errors reporting wrong line numbers Fix script errors reporting wrong line numbers Oct 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants