Compile fixes by csware · Pull Request #184 · ProgerXP/Notepad2e

csware · 2018-08-15T07:26:10Z

Ref. issue #178

Signed-off-by: Sven Strickroth <email@cs-ware.de>

ProgerXP · 2018-08-15T08:08:11Z

Thank you for this. We will review it.

@cshnik Review it too and comment if something is off. The end goal is to make it compiling on any combination of x86/x64/Release/Debug, including tests (all projects in the solution), and using more uptodate settings than came from Notepad2. And it should still run on XP x86 SP3/x64 SP2.

cshnik · 2018-08-15T18:21:40Z

- </PrecompiledHeader>
- <WarningLevel>Level3</WarningLevel>
- <DebugInformationFormat>ProgramDatabase</DebugInformationFormat>
+ <PreprocessorDefinitions>_M_IX86;%(PreprocessorDefinitions)</PreprocessorDefinitions>


In my tests _M_IX86, _M_AMD64 macroses are excess and does not affect the build process. These are Microsoft-specific predefined macroses: https://msdn.microsoft.com/en-us/library/b0084kay.aspx. I assume we can completely get rid of them.

I kept it as this was also explicitly set in the old version. Won't hurt.

_M_IX86 macro caused x64 build to fail. In your configurations it was replaced with _M_AMD64. However there is also a direct alternative _M_IX86 (x86) for x64: _M_X64.
Are there any thoughts why should we retain any of these in renovated configurations?

_M_IX86 is not set for x64 builds (it was, but now it isn't any more)!

_M_AMD64 is used in version.h

_M_IX86/_M_AMD64 are the predefined compiler macros which should not be explicitly defined in the project options. File version.h is used to define data shown on the binary details tab and in the application's about dialog; currently, "x64"-postfix depends on the _M_AMD64 macro.
The actual problem here is that resource compiler (RC) know nothing about the compiler provided macros which in its turn requires to directly define _M_AMD64 in the project to allow RC generate correct data.
Since your fixes for x64 are already large I offer to include an additional fix for this case to make project smooth and clear.
Get rid of compiler flags _M_IX86/_M_AMD64
Add _WIN64 definition for x64 configurations
Adjust version.h: replace _M_AMD64 with _WIN64
Does it sound reasonable?

_WIN64 is also automatically added by the compiler, so no need for this to add manually.

_WIN64 is also automatically added by the compiler, so no need for this to add manually.
The actual problem here is that resource compiler (RC) know nothing about the compiler provided macros which in its turn requires to directly define _WIN64 in the project to allow RC generate correct data.

Ah, I see, your point 2 was for the resource compiler and not the PreprocessorDefinitions

Signed-off-by: Sven Strickroth <email@cs-ware.de>

…utionPrevention and RandomizedBaseAddress Signed-off-by: Sven Strickroth <email@cs-ware.de>

Signed-off-by: Sven Strickroth <email@cs-ware.de>

csware · 2018-08-15T21:13:36Z

Why still support WIndows XP? XP ran out of support several years ago, it's kinda dangerous to run it.

ProgerXP · 2018-08-15T21:26:15Z

Why still support WIndows XP? XP ran out of support several years ago, it's kinda dangerous to run it.

Running an unpatched (that's what you mean by unsupported?) OS may or may not be more dangerous than running latest Win7/8/10, it depends on the user. It's like saying that having antivirus installed will protect you from all threats (so far they have failed in preventing even major epidemics).

Then again, the POS version of XP is supported until next year (or maybe they have prolonged it again?). And security there is probably even more important to MS then security of its desktop OSes so I get it should be still providing all the security patches...

Speaking seriously though, I still reasons to support XP: there is a good number of people running it on older machines (it still has a decent Firefox version available, among other software), and second, XP may be used in virtual environment. (Third, VS 2017 still has support for XP, which is unbelievable and speaks for itself.)

And anyway, the program was written for XP and I see no reasons to drop XP support. It's up to the user if he wants to run it or not, it doesn't make the program itself more vulnerable.

cshnik · 2018-08-15T21:57:02Z

@ProgerXP PR is ready for merge.

csware · 2018-08-15T22:49:28Z

XP support in VS2017 is kinda broken.

cshnik · 2018-08-16T17:10:15Z

XP support in VS2017 is kinda broken.

I've checked test builds Debug/Release x86/x64 on clean Windows XP x64 and the only problem is Release x64 crashed when calling Open file dialog. I suppose this issue can be fixed separately from this PR.

cshnik · 2018-08-16T17:40:50Z

the only problem is Release x64 crashed when calling Open file dialog

The bug is in n2e_SubclassOpenDialog():

SetProp(hwnd, PROPERTY_ORIGINAL_WINDOW_PROC, (HANDLE)SetWindowLongPtr(hwnd, GWLP_WNDPROC, (long)n2e_OpenDialogWndProc));

should be replaced with:

SetProp(hwnd, PROPERTY_ORIGINAL_WINDOW_PROC, (HANDLE)SetWindowLongPtr(hwnd, GWLP_WNDPROC, (LONG_PTR)n2e_OpenDialogWndProc));

ProgerXP · 2018-08-16T20:09:55Z

Let's leave alone the wdk folder. It isn't the cause for failed compilation as far as I understand. Even if it's outdated, it should be written so in the README rather than removed so that people don't start questioning its absence here, go looking for wdk in the original project and/or we can get to update it at some point in time.

I am ready to merge this when wdkbuild/Notepad2.mak and wdkbuild/make.cmd are restored.

_CRT_SECURE_NO_WARNINGS

What does this definition do (it's in Notepad2 too)? Do we have unsafe operations that need silencing?

ProgerXP · 2018-08-16T20:20:02Z

@cshnik

The bug is in n2e_SubclassOpenDialog():

Was this fix part of this pull? If not was this committed to master?

csware · 2018-08-17T07:31:20Z

What does this definition do (it's in Notepad2 too)? Do we have unsafe operations that need silencing?

Yes, there are many.

The bug is in n2e_SubclassOpenDialog():

This is also in the PR.

Let's leave alone the wdk folder. It isn't the cause for failed compilation as far as I understand.

When you compile using that file it will fail. I don't understand why you want to keep it. It makes thinks just more complicated for people.

ProgerXP · 2018-08-17T07:44:04Z

When you compile using that file it will fail. I don't understand why you want to keep it. It makes thinks just more complicated for people.

Using sln/vcxproj from this repo without changes won't cause problems, right? Then updating or removing wdkbuild is a different issue from this PR.

We went a good length to preserve the structure of the original project so that it is a drop-in replacement not just in runtime but in the sources. If whoever needed wdkbuild still needs it, he'll find it here. But the docs should be updated, this is true.

csware · 2018-08-17T07:53:06Z

What's the advantage of keeping the original sources intact? - There are already lots of changes in them. - Besides this, there is the version history.

if whoever needed wdkbuild still needs it, he'll find it here.

Do you think there are lots of other developers who work on this project? if they still use wdkbuild then they must have already modified it...

Using sln/vcxproj from this repo without changes won't cause problems, right?

Sorry, don't understand.

ProgerXP · 2018-08-17T08:21:05Z

Using sln/vcxproj from this repo without changes won't cause problems, right?

Sorry, don't understand.

If we keep wdkbuild files, they won't cause any issues with compilation unless you actually try to use them, right? Because our default project settings don't reference them.

csware · 2018-08-17T08:57:24Z

If we keep wdkbuild files, they won't cause any issues with compilation unless you actually try to use them, right? Because our default project settings don't reference them.

But why to keep them? Even if you document somewhere, people will not see it and try...

ProgerXP · 2018-08-17T09:04:26Z

Do we at least agree that manipulations on wdkbuild are not vital for fixing our immediate compilation issues and so are part of another issue or PR?

csware · 2018-08-17T09:05:29Z

I put it into this issue as compiling using wdkbuild will fail.

Signed-off-by: Sven Strickroth <email@cs-ware.de>

Add Notepad2evcxproj.filters
d4f427a
Signed-off-by: Sven Strickroth <email@cs-ware.de>

csware force-pushed the compile-fixes branch 2 times, most recently from ad08562 to 25f9202Compare August 15, 2018 07:42

csware mentioned this pull request Aug 15, 2018
Optimize for size #185
Open

cshnik reviewed Aug 15, 2018
View reviewed changes

csware added 7 commits August 15, 2018 23:05

Optimize VS project files
c4278b2
Signed-off-by: Sven Strickroth <email@cs-ware.de>

Fix build in VS2017: Don't use precompiled headers
128ad04

Add .gitignore
955e72d
Signed-off-by: Sven Strickroth <email@cs-ware.de>

Allow building with VS2015 and VS2017 at the same time
59e978e
Signed-off-by: Sven Strickroth <email@cs-ware.de>

Improve security by explicitly enabling BufferSecurityCheck, DataExec…
cebe60b
…utionPrevention and RandomizedBaseAddress Signed-off-by: Sven Strickroth <email@cs-ware.de>

Improve optimization
8b829ad
Signed-off-by: Sven Strickroth <email@cs-ware.de>

Fix compilation of x64 version
d5bf5a2
Signed-off-by: Sven Strickroth <email@cs-ware.de>

csware force-pushed the compile-fixes branch from ebbc8c2 to 4d5bdfbCompare August 15, 2018 21:06

Don't build Notepad2eTests in Release configuration
8f0746c
Signed-off-by: Sven Strickroth <email@cs-ware.de>

Don't define buildin compiler defines
0e63229
Signed-off-by: Sven Strickroth <email@cs-ware.de>

csware force-pushed the compile-fixes branch from 4d5bdfb to 0e63229Compare August 17, 2018 09:07

ProgerXP merged commit 39e9865 into ProgerXP:masterAug 17, 2018

This was referenced Aug 17, 2018
Check 64-bit pointer arithmetics #188
Open
Does not compile with VS2017 #178
Closed

csware deleted the compile-fixes branch August 21, 2018 07:41

Conversation

csware commented Aug 15, 2018

Uh oh!

ProgerXP commented Aug 15, 2018

Uh oh!

cshnikAug 15, 2018

Choose a reason for hiding this comment

Uh oh!

cswareAug 15, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cshnikAug 15, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cswareAug 15, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cswareAug 15, 2018

Choose a reason for hiding this comment

Uh oh!

cshnikAug 15, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cswareAug 15, 2018

Choose a reason for hiding this comment

Uh oh!

cshnikAug 15, 2018

Choose a reason for hiding this comment

Uh oh!

cswareAug 15, 2018

Choose a reason for hiding this comment

Uh oh!

csware commented Aug 15, 2018

Uh oh!

ProgerXP commented Aug 15, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cshnik commented Aug 15, 2018

Uh oh!

csware commented Aug 15, 2018

Uh oh!

cshnik commented Aug 16, 2018

Uh oh!

cshnik commented Aug 16, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ProgerXP commented Aug 16, 2018

Uh oh!

ProgerXP commented Aug 16, 2018

Uh oh!

csware commented Aug 17, 2018

Uh oh!

ProgerXP commented Aug 17, 2018

Uh oh!

csware commented Aug 17, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ProgerXP commented Aug 17, 2018

Uh oh!

csware commented Aug 17, 2018

Uh oh!

ProgerXP commented Aug 17, 2018

Uh oh!

csware commented Aug 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cswareAug 15, 2018•
edited
Loading

cshnikAug 15, 2018•
edited
Loading

cswareAug 15, 2018•
edited
Loading

cshnikAug 15, 2018•
edited
Loading

ProgerXP commented Aug 15, 2018•
edited
Loading

cshnik commented Aug 16, 2018•
edited
Loading

csware commented Aug 17, 2018•
edited
Loading