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

Fixed C# project module "AGS.CScript.Compiler" so that it successfully builds in xBuild on *nix systems #406

Merged

Conversation

persn
Copy link
Contributor

@persn persn commented May 5, 2017

Needs #396 to be resolved in order to test properly since AGS.Cscript.Compiler references AGS.Types

if (helpHandle == null)
{
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this meant to compare with 0 (zero handle) instead.

Quote from MSDN:

A process has a main window associated with it only if the process has a graphical interface. If the associated process does not have a main window, the MainWindowHandle value is zero. The value is also zero for processes that have been hidden, that is, processes that are not visible in the taskbar. This can be the case for processes that appear as icons in the notification area, at the far right of the taskbar.

@ghost
Copy link

ghost commented May 13, 2017

I checked this out, and found just two changes that need to be reworked in my opinion.

Regarding unused _isLegacyString flag, I do know what that means, but since this compiler is not complete, I cannot tell how it was supposed to be used. If anyone will finish this in the future, they would have to address this issue anyway (assuming legacy strings will still be supported, which is questionable).

@ghost
Copy link

ghost commented May 13, 2017

E: actually, I rejected one of my comments, noticing that the change only affects local reader object.

persn added 5 commits May 17, 2017 14:17
Variable "setEqualTo" is assigned, but value is never used. I quite simply removed the variable assignment. We still need to call the function, as quite lot is happening in the code behind.

Variable "memberName" is assigned, but value is never used. There are several TODOs in the code surrounding this variable. I simply commented the variable in case of future needs.
Unused variable. I couldn't find that it did anything and removing it doesn't seem to break anything obvious.
Unused variable. This one seem to had a great deal of thought put into it with a full Enum list to go with it. I decided to fix this by adding a getter just in case.
IntPtr is value type which means it will never be null. We check for IntPtr.Zero instead
Unused variable. Removed it but kept the function call that assigned the variable as there is stuff happening in the code-behind.
@persn persn force-pushed the hotfix/ags-cscript-nix-build branch from eda78e3 to f76099f Compare May 17, 2017 12:50
@persn
Copy link
Contributor Author

persn commented May 17, 2017

Updated according to feedback and rebased on master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants