htacg / tidy-html5

The granddaddy of HTML tools, with support for modern standards
http://www.html-tidy.org
2.71k stars 418 forks source link

`sprtf` library should work cross platform #604

Closed balthisar closed 6 years ago

balthisar commented 7 years ago

Although this is intended for debugging, its features should be platform agnostic. It doesn't affect release builds of Tidy, so this isn't a hard milestone.

geoffmcl commented 7 years ago

@balthisar I have been using my little sprtf module for a long, LONG time, and for sure it should be platform agnostic...

I have not yet had time to fully check whether tidy has my latest sprtf.[c|h] modules, but a quick review indicates a 2017/02/12 date, which is for sure fairly recent, but advise if any problem compiling it in unix/macOS as I do have versions that work fine in several unix projects...

In tidy code, the main thing seems to be replacing the clumsy _MSC_VER macro, #if !defined(NDEBUG) && defined(_MSC_VER), and some variations, with something more appropriate...

As advised, will help where I can... thanks...

balthisar commented 6 years ago

Candidate #626.

balthisar commented 6 years ago

Another one closed! On a roll...

geoffmcl commented 6 years ago

@balthisar yes, you are certainly On a roll... ;=))

So fast in fact that I did not have time to respond on this...

My concern is that all you have done mainly is to remove the defined(MSC_VER)...

On many that is fine because they have a second define, like defined(DEBUG_PPRINT), defined(DEBUG_INDENT), defined(DEBUG_MEMORY), or defined(DEBUG_ALLOCATION), and that is ok...

But that leaves many with just #if !defined(NDEBUG). Now that is also fine in the Windows build because NDEBUG is automatically added to the MSVC builds other than the Debug configuration...

But this is not the case in linux. If the user does not add -DCMAKE_BUILD_TYPE=Release, then I have seen cases where NDEBUG is not defined automatically...

I am just afraid linux users could end up seeing all this Debug ONLY output from the EXE they build... and I am sure they would be surprised, if not shocked by all the extra output, that I am sure they do not want to see...

I have not thought out a solution yet... and maybe my concern is unwarranted... but I would certainly feel better about the macro being changed to something that the user has to opt-in to use... not get accidentally...

And maybe there is a case for also being able to enable it in other than the Windows Debug build... that is none of it depends just on NDEBUG...

Will give this some thought, and maybe you have an idea... thanks...

balthisar commented 6 years ago

@geoffmcl, good point. It wasn't an issue on Ubuntu, but that's admittedly a small sample size. Perhaps a new define controlled in tidyplatform and cmakelists?

geoffmcl commented 6 years ago

@balthisar not sure what test you did in Ubuntu, but as predicted, if I just do -

$ cat version.txt
5.5.59
2017.10.06
$ cd build/cmake
$ cmake ../..
$ make
$ cat ../../version.txt > temp.html
$ ./tidy temp.html

That is without the option -DCMAKE_BUILD_TYPE=Release, the resultant tidy will output all the SPRTF debug stuff... very alarming if you have not seen it before... totally unexpected...

But could not find the ex.log - note: need to change this default in sprtf.c... but did find the ouput sent to ex.log.html... this need to be fixed in case set_log_file(...) is not called... but why no ex.log... puzzling... need to look at macro switches again in tidy.c...

And if I turn on -DCMAKE_VERBOSE_MAKEFILE=ON, and look carefully at the cc compile lines I can not find -DNDEBUG added... seem the Ubuntu cmake has no default build type, or it defaults to Debug... need to check more on this...

Thinking more about it, maybe replace all, well most - there may be some exceptions - need to check - #if !defined(NDEBUG) with say #if defined(DEBUG_TIDY), or something... still contemplating...

I would also add it as an option in CMakeLists.txt, so it can be turned on anytime... for any build type...

As to whether there should be some default in say tidyplatform.h not sure... How could it be done and still avoiding the problem if CMAKE_BUILD_TYPE is not defined by the user?

Maybe it could again be based on MSC_VER, since, as indicated, the MSVC solution files will always add -DNDEBUG in all builds except Debug... But this is mainly for my convenience, in that I do want this Debug stuff on for the Debug build, hopefully by default... as I had...

Still pondering, and experimenting... but feel something should be urgently done for the *nix builds, before we get the expect WTF message ;=))

What would be your idea? Thanks...

balthisar commented 6 years ago

I usually test with release, and given our build instructions don't mention that, I think you're right!

Let me push a hot fix in the next 45 minutes or so, since I screwed it up.

with say #if defined(DEBUG_TIDY)

Yup, this is what I'm going to do.

geoffmcl commented 6 years ago

@balthisar certainly agree with the hot fix using DEBUG_TIDY... thanks...

And check the Before: and After: showing the flags as below. I seem to get this output even in the Windows Release...

Before: HT20|HT32|H40S|H40T|H40F|H41S|H41T|H41F|X10S|X10T|X10F|XH11|XB10|----|HT50|XH50
After : ----|HT32|H40S|H40T|H40F|H41S|H41T|H41F|X10S|X10T|X10F|XH11|XB10|----|HT50|XH50
line 1 column 1 - Warning: missing <!DOCTYPE> declaration
line 1 column 1 - Warning: inserting implicit <body>
After : ----|HT32|H40S|H40T|----|H41S|H41T|----|X10S|X10T|----|XH11|XB10|----|HT50|XH50
line 1 column 1 - Warning: inserting missing 'title' element
Info: Document content looks like HTML5
Tidy found 3 warnings and 0 errors!

And maybe adjust the default file name in sprtf.c, if you get a chance... would suggest temptidy.txt for all cases... thanks...

geoffmcl commented 6 years ago

Yeah, the Before: After : stuff, Line 325 should also be changed to #if defined(DEBUG_TIDY)... thanks...

PS: Will shortly be out for a while, and may not get back to my machines tonight... my friend has a black blank screen and is screaming for help...

balthisar commented 6 years ago

The "hot fix" was fairly easy, in that I simply tweak NDEBUG manually. This fixes code; will go back with more time and implement DEBUG_TIDY or similar.

balthisar commented 6 years ago

Okay, I've added ENABLE_DEBUG_LOG as a substitute, and it's live. Preserves behavior on Windows, and no nasty surprises when others are following our build instructions.

geoffmcl commented 6 years ago

@balthisar thanks for the hot fix, but nope! Now I get ENABLE_DEBUG_LOG enabled all-the-time in Windows...

Still checking why, but it seems CMAKE_BUILD_TYPE is not defined when I build in windows, so now the CMakeLists.txt enables it all the time... I do not define it when I run cmake ..\.. options in Windows...

So I think preserving my Windows behaviour, to get logging for the Debug build only, must be done outside cmake...

Still looking for HOW!

balthisar commented 6 years ago

@geoffmcl, huh? Does if (MSVC AND NOT CMAKE_BUILD_TYPE MATCHES Release) not catch it? That is, I thought your preferred behavior was debug out by default, which is why I added that line for Windows. If I build Release specifically, then there's no debug output.

Right now it's a Windows-specific behavior; it would be trivial to make it so that all platforms require -DENABLE_DEBUG_LOG.

Sorry, thought I was trying to preserve your preference...

geoffmcl commented 6 years ago

@balthisar yes, understand what you were trying to do, and thanks... but, in general, cmake can only define global macros. Not macros that change depending on the build configuration...

As stated I run cmake without setting CMAKE_BUILD_TYPE, eg -

$ cmake ..\.. [options]
$ cmake --build . --config debug
$ cmake --build . --config release

So agree, the option ENABLE_DEBUG_LOG must be default off for all platforms... please push this if you get a chance...

It may be difficult to have this automatic difference in Windows, where Debug and Release are always built by me...

Maybe something like, in tidyplatform.h -

#if defined(_MSC_VER) && !defined(ENABLE_DEBUG_LOG) && !defined(NDEBUG)
#define ENABLE_DEBUG_LOG
#endif

Or something... still searching... ideas welcome... thanks...

balthisar commented 6 years ago

Oh! I see! cmake --build is doing something different on Windows than make does on Unix, or I'm using cmake wrong. I always cmake my configuration with CMAKE_BUILD_TYPE prior to making, but you're suggesting that you only cmake, and then cmake --build different configurations.

I consider myself "schooled" on this point.

Although I'm fine with cmake ../.. -DENABLE_DEBUG_LOG prior to make, I think that's not your workflow. I'll add a hotfix to force it this way for an immediate fix, but what's your workflow, so that I can try to accommodate it? Is this correct?

Given that I config, then make, I'm not sure where cmake --build on Windows gets the configuration differences for the --config argument. I'm guessing they may be present on Unix, too, and that this is a missing piece of knowledge to me.

geoffmcl commented 6 years ago

@balthisar WOW, out of the fat, and into the fire ;=))

Now you have defaulted the cmake option ENABLE_DEBUG_LOG to ON for all platforms??? Why would you do that? Is that what you intended? I think this should default to OFF for all...

And as an aside, I wish the cmake option and the macro had different names, just to avoid confusion in discussion, as I have done with say option ENABLE_MEMORY_DEBUG, versus macro DEBUG_MEMORY, etc...

And that points out another problem, now you have added option ENABLE_DEBUG_LOG... if you want option ENABLE_MEMORY_DEBUG, you can only have that IFF you also enable ENABLE_DEBUG_LOG. A problem I did not have with how I have it set up for just _MSC_VER... This needs to be thought out again...

I just wish we were experimenting with all this in a branch, but in future I would ask a little more time, patience, before merging a PR... 48 hours is not sufficient time... no time for a review, especially when it effects so much, so many files, builds, etc... ... but that is water under the bridge...

Ok, we have to separate the two ports - unix versus windows. Of course they are, using the default cmake generator, very different! In unix, a standard make Makefile is generated, and in Windows, if you have MSVC installed, a MSVC solution file is generated. In both systems there are alternative generators. The Windows list is quite large -

  Visual Studio 14 2015 [arch] = Generates Visual Studio 2015 project files.
                                 Optional [arch] can be "Win64" or "ARM".
  Visual Studio 12 2013 [arch] = Generates Visual Studio 2013 project files.
                                 Optional [arch] can be "Win64" or "ARM".
  Visual Studio 11 2012 [arch] = Generates Visual Studio 2012 project files.
                                 Optional [arch] can be "Win64" or "ARM".
  Visual Studio 10 2010 [arch] = Generates Visual Studio 2010 project files.
                                 Optional [arch] can be "Win64" or "IA64".
  Visual Studio 9 2008 [arch]  = Generates Visual Studio 2008 project files.
                                 Optional [arch] can be "Win64" or "IA64".
  Visual Studio 8 2005 [arch]  = Generates Visual Studio 2005 project files.
                                 Optional [arch] can be "Win64".
  Visual Studio 7 .NET 2003    = Generates Visual Studio .NET 2003 project
                                 files.
  Visual Studio 7              = Deprecated.  Generates Visual Studio .NET
                                 2002 project files.
  Visual Studio 6              = Deprecated.  Generates Visual Studio 6
                                 project files.
  Borland Makefiles            = Generates Borland makefiles.
  NMake Makefiles              = Generates NMake makefiles.
  NMake Makefiles JOM          = Generates JOM makefiles.
  Green Hills MULTI            = Generates Green Hills MULTI files
                                 (experimental, work-in-progress).
  MSYS Makefiles               = Generates MSYS makefiles.
  MinGW Makefiles              = Generates a make file for use with
                                 mingw32-make.
  Unix Makefiles               = Generates standard UNIX makefiles.
  Ninja                        = Generates build.ninja files.
  Watcom WMake                 = Generates Watcom WMake makefiles.
  CodeBlocks - MinGW Makefiles = Generates CodeBlocks project files.
  CodeBlocks - NMake Makefiles = Generates CodeBlocks project files.
  CodeBlocks - Ninja           = Generates CodeBlocks project files.
  CodeBlocks - Unix Makefiles  = Generates CodeBlocks project files.
  CodeLite - MinGW Makefiles   = Generates CodeLite project files.
  CodeLite - NMake Makefiles   = Generates CodeLite project files.
  CodeLite - Ninja             = Generates CodeLite project files.
  CodeLite - Unix Makefiles    = Generates CodeLite project files.
  Eclipse CDT4 - MinGW Makefiles
                               = Generates Eclipse CDT 4.0 project files.
  Eclipse CDT4 - NMake Makefiles
                               = Generates Eclipse CDT 4.0 project files.
  Eclipse CDT4 - Ninja         = Generates Eclipse CDT 4.0 project files.
  Eclipse CDT4 - Unix Makefiles= Generates Eclipse CDT 4.0 project files.
  Kate - MinGW Makefiles       = Generates Kate project files.
  Kate - NMake Makefiles       = Generates Kate project files.
  Kate - Ninja                 = Generates Kate project files.
  Kate - Unix Makefiles        = Generates Kate project files.
  Sublime Text 2 - MinGW Makefiles
                               = Generates Sublime Text 2 project files.
  Sublime Text 2 - NMake Makefiles
                               = Generates Sublime Text 2 project files.
  Sublime Text 2 - Ninja       = Generates Sublime Text 2 project files.
  Sublime Text 2 - Unix Makefiles
                               = Generates Sublime Text 2 project files.

Windows ONLY

I am sure you understand some of this, and really sorry for the repetition, but it seems important to exactly establish the situation...

My 'standard' cmake command line is -

Doing: 'cmake ..\.. -DCMAKE_INSTALL_PREFIX=..\..\..\software.x64 -G "Visual Studio 14 Win64" \
 -DBUILD_SHARED_LIB:BOOL=OFF' 

But remember that build file generation can also be done in the CMake GUI, when you will be asked to set the generator...

And by default, the Visual Studio generation will generate FOUR(4) configurations - Release, Debug, RelWithDebInfo, MinSizeRel, but that can be configured, changed...

Now the actual build can be done in the MSVC IDE GUI... just load it on the Tidy.sln file, select the desired configuration, and build... Ctrl+Shift+B... if you select one after the other, you can build 4 version of Tidy.exe, output to different paths...

OR you can use a command line, like I do... which runs the MS Build Engine, MSbuild.exe? IIRC, invoked by -

$ cmake --build . --config <CONFIG>

And the <CONFIG> type can be any of the 4... And the tidy.exe built is placed in an appropriate output directory, to keep each build separate...

Unix/Linux

The above is very foreign to unix, which seems to only build ONE configuration at a time, writing a single executable, tidy, in the build directory, using make...

Although, as in Windows cmake, there are some 14 or so generators, only one of which is Unix Makefiles... seemingly the default, assuming you have the appropriate tools installed...

And maybe it only has two(2) - Release or Debug... and as I understand it, even the Debug is only an EXE with symbols embedded, and can in fact be stripped... so is the same or similar to the release, except perhaps for optimization...

Now in a quick test, it seems $ cmake --build . --config <CONFIG>, can also be used to run make, but given that the resultant tidy EXE has the same size, I guess building different configuration versions this way fails with `make'.

The actual Makefile generated must be different for Release and Debug, so that choice must be made in the actual generation phase... established by -DCMAKE_BUILD_TYPE=<config>...

And it is still up-for-grabs which is built if you do not add that option...

Summary

So, yes, I would like to maintain the default I have had for YEARS, but that is not the important issue... I will continue to search for that... but only when all builds, in all systems settle back to what we had...

Namely, a default build in all, using the minimum of option choices, produces a useable Release EXE...

AND that should cover all possible generators available in that system...

So back here, late at night, pondering this... the single important issue seems to be to default that option OFF... have pushed this... and bumped to 5.5.63...

Will need some time, and help to address other items... as always, ideas welcome... thanks...

balthisar commented 6 years ago

Now you have defaulted the cmake option ENABLE_DEBUG_LOG to ON for all platforms??? Why would you do that? Is that what you intended? I think this should default to OFF for all...

Wait, WTF???? Yes, it should be OFF; did I screw up a local merge???

So back here, late at night, pondering this... the single important issue seems to be to default that option OFF... have pushed this... and bumped to 5.5.63...

Yes, thank you!!!

I will digest everything else...

balthisar commented 6 years ago

@geoffmcl, thanks for the in-depth explanation of the Windows build process. When I'm in the Windows world, I've actually built .sln files just so that I could build in Visual Studio 2010; I had no idea that this was essentially the default.

Our README indicates this:

cmake ../.. -DCMAKE_BUILD_TYPE=Release [-DCMAKE_INSTALL_PREFIX=/path/for/install]

... which really mislead me into thinking it was similar to the Unix way. I actually kind of like the fact that you can configure once, and build multiple targets (actually, I get that in Xcode, but not when building with make).

I will study this issue in-depth before making any more changes; it's a lot more complex than it seemed at first.

geoffmcl commented 6 years ago

@balthisar on further reflection, after a good nights sleep, while I was initially all for expanding this DEBUG capability to *nix systems, it is clear now that this is not very convenient for Windows MSVC, and maybe XCode, developers, in that the various configuation build types are done very differently.

Now, if you turn on -DENABLE_DEBUG_LOG:BOOL=ON, in the cmake configuration and generation stage you get the extra DEBUG output in ALL configurations, including Release. As a Windows developer, I am not sure I would ever want that!

So maybe !defined(NDEBUG) should still be involved? This would mean devs, in all ports, could never enable it for a Release configuration build! But at this moment, do not see that as a bad thing!

Need to reflect more... I am a slow thinker, and want to thoroughly test... of course, I would move this testing back to a branch, maybe issue-604 to get it right...

While we agree, the next branch is for development, but I would like to keep it 100% stable and usable to encourage more people to fully trust it, like a release...

What are your thoughts, or the thoughts of others on this? Thanks...

Concerning the README/BUILD.md docs, as always, I agree this could do with some enhancements. Ideas welcome...

And then there is the question of the default log file name. I just copied my sprtf modules from another project called ex, but added 'set_log_file((char *)"temptidy.txt", 0);`, but that is not good for library users, who should not need to add this to their apps...

That's an easy change in sprtf.c but what name should we use?

For windows I chose a local to the runtime directory. Again that is very convenient for running in the MSVC IDE Debugger, which uses the solution directory by default, and as you will see the root .gitignore contains temp*, so it was easy to find and read, and ignored by git. So here, want to keep temptidy.txt for Windows.

But this is not the usual place, or name, for *nix. I suppose we could use something like /tmp/tidy.log. But on the other hand, a *nix developer who enabled this extra debug output should not be installing that version of tidy. They should be just testing that special debug tidy in the build directory only. So again, maybe a local temptidy.txt is appropriate in that case...

Again, look forward to further comments? Thanks...

And as a separate issue, just built such a debug tidy, 5.5.63, with symbols as well, and could not find an ex.log? Why is this not being written?

Tried to run it using gdb... set break sprtf, then run, but it does not break!!!

I have tried gdb many times before, but never seem to be able to get what I want! Does anybody know a good GUI debug for Ubuntu that I can try?

And perhaps others with more gdb skill can help out here... why is no log file written? Or at least give me the commands. I did -

$ gdb --args ./tidy -xml tidy-help.xml
GNU gdb (Ubuntu 7.7.1-0ubuntu5~14.04.2) 7.7.1
... skipped ...
Reading symbols from ./tidy... done.
(gdb) break sprtf
Breakpoint 1 at 0x41c8b0: file ...sprtf.c, line 371
(gdb) run
... lots of output, including the sprtf debug stuff
[Inferior 1 (process 3742) exited normally]
(gdb)

What am I doing wrong? Thanks...

balthisar commented 6 years ago

@geoffmcl, oh, what a can of worms I opened!

I agree, let's take this to another branch.

In the CMakeLists.txt, I think we can set it up so that if the other debug options are enabled, we can infer that ENABLE_DEBUG_LOG is implied. So that's one chink out of the way.

As for convenience, here's what I do on macOS: I cmake with the desired flags before every build. Typically I only do this when running the regression suite. Otherwise for debugging, I use a self-generated project rather than the rather dirty project that CMake generates for XCode. I have multiple targets for whatever options I'm building in, such as logs. It's trivial to use one or the other for me; I just choose one.

Given that we're really the only two active developers, I would tend to err on the side of "least surprise," i.e., if someone follows our build instructions, they get a working Tidy without debug messages. I'm certain that you agree. So, then, how to we simply enable debug output for people who make a deliberate choice to contribute code, while at the same time not handicapping our own workflows (which is what I'm afraid I did to you)?

If someone goes to great pains to enable debug output in a Release build, I don't see the point of stopping them; it's a deliberate act. It would be nice to prevent it, but I don't see it as a priority.

Default name, we could get from the version number; might be convenient to you if you add the RC_NUMBER field. Otherwise, I'm ambivalent.

For me, the runtime directory works great for make or Xcode. With make, it's in the build directory, and with Xcode it's in the out-of-source-build directory, exactly where I expect it.

I'll try to deep dive this on the Mac/Linux front!

geoffmcl commented 6 years ago

@balthisar have not had time to explore all here, but think I have at least found why the log file is not written in Ubuntu...

As part of commit dedcb7bb4d, the definition of SPRTF was removed from sprtf.h!!! WHY???

diff --git a/src/sprtf.h b/src/sprtf.h
index 14c3e02..c589707 100644
--- a/src/sprtf.h
+++ b/src/sprtf.h
@@ -62,10 +62,6 @@ TIDY_EXPORT char *get_date_time_stg();
 TIDY_EXPORT int gettimeofday(struct timeval *tp, void *tzp);
 #endif

-#ifndef SPRTF
-#  define SPRTF sprtf
-#endif
-
 #ifdef   __cplusplus
 }
 #endif

Without this, in other places SPRTF is just defined as printf... I only added that in case any use of SPRTF was outside the original macro #if !defined(NDEBUG) && defined(_MSC_VER)... There should be none, and that define to printf should be removed now that sprtf.[h|c] is included in all ports...

And agreed, it is trivial to only accept the additional debug options, if the primary is on, or at least warn and/or advise that otherwise say if ENABLE_ALLOC_DEBUG is set by the user, it will do nothing... this should certainly be added/done one way or another...

And I do not particularly want to somehow stop enabling the extra debug in the Release build. As you point out that is their deliberate choice...

The only thing I would like is to be able to add it only in the Debug configuration, by default if possible, IFF MSVC... as I had, but as stated, not important, and will work on that...

The main thing now is to even get the log file written, in all ports, if ENABLE_DEBUG_LOG is ON...

Will try to work on solutions here, as time permits, but would be great if you beat me to it... thanks...

geoffmcl commented 6 years ago

@balthisar well to be very frank it is not a can of worms you opened!, but more what you created in merging the sprtf branch without fully testing, or waiting for a review ;=))

Now I see more potential problems! You have included sprtf.h in tidyplatform.h, but sprtf.h is not a header included in the distribution, nor should it be, so a libtidy user will potentially not be able to link with libtidy in their app...

They will include tidy.h, and if they define ENABLE_DEBUG_LOG accidently or experimently or for what ever reason, they will find they are missing an include. That should not be the case...

If we do want to add SPRTF to the distribution, and I suggest not, then at the very least sprtf.h should be renamed to say tidysprtf.h to avoid any generic clash of names... But I obviously think distributing it is a BAD idea... aside from the fact that the extra debug stuff would not be in the library anyway...

In essence, we should never be distributing a libtidy with this enabled. So it should have no meaning if accidentally enabled in someone's 3rdParty app... they would be linking to a libtidy that does not contain this extra debug stuff anyway... full stop!

I do not mind the restriction that ENABLE_DEBUG_LOG can only be done if you are building the library from source...

To me the only use of this extra debug stuff, is if you are DEBUGGING a potential problem in libtidy, before it is distributed... and really helped me recently to track down the problem #597, and several other items before this... it is also a learning/teaching tool... thus sprtf.h should only be available in the source!

My fixes would include essentially moving ENABLE_DEBUG_LOG out of tidyplatform.h... sprtf.h would only be included in modules that use SPRTF, and defining SPRTF as sprtf, if not defined, not printf...

Before I start making fixes, in perhaps an issue-604 branch, to close this can!, are we in full agreement on this? Or am I misunderstanding something? Missed something else you have in mind? Please advise... thanks...

geoffmcl commented 6 years ago

@balthisar pushed initial cut to issue-604 branch... looks good in Windows MSVC... still to test in Ubuntu...

Other testing welcome... thanks...

geoffmcl commented 6 years ago

Have now pushed another small fix to the issue-604 branch to not include the sprtf code unless ENABLE_DEBUG_LOG is ON... and re-tested in Windows 10...

Note, have added back the auto-building of ENABLE_DEBUG_LOG in the MSVC Debug configuration, that I had, but this can be disabled by defining DISABLE_DEBUG_LOG...

Have now further tested it in Ubuntu 14.04, and in Raspbian GNU/Linux 8.0 (jessie), and all appears to work fine...

Conveniently, issue #631, @jokester PR #630 provides a convenient memory leak test. I have a perl script, shwtidymem.pl, which will analyse the temptidy.txt log file, and correctly report the memory leak...

Thus in a build/temp-604 did the following... Note, it is using the Release build in this case, but could just as easily been the Debug build...

$ cmake ../..  -DCMAKE_INSTALL_PREFIX=/usr -DENABLE_DEBUG_LOG:BOOL=ON \
-DENABLE_ALLOC_DEBUG:BOOL=ON -DENABLE_MEMORY_DEBUG:BOOL=ON \
-DTIDY_RC_NUMBER=I604 -DCMAKE_BUILD_TYPE=Release'
... skipped ...
$ echo "Hello World" | ./tidy
... skipped ...
$ shwtidymem.pl temptidy.txt
... skipped ...
262:102: FREE 0x20fc008 6924 1
Found 104 alloc, 0 realloc, and 102 frees...
Total mem 129668, Max mem 29635
Remains 2 in HASH
Ordered list...
# :cnt: address          size
 1:  7: 0x20fe660 24
 2:  9: 0x20fe708 256

Note it clearly shows the memory leaks... real easy...

Now I would/could share the shwtidymem.pl script, but it also requires my lib_utils.pl, but must find a place to put these... suggestion welcome...

So this has now been tested in Windows 10, Ubuntu and Raspbian, and all appears ok... created PR #632...

If others could review/test would merge that PR... thanks...

jokester commented 6 years ago

Didn't know there are options like ENABLE_DEBUG_LOG (#630 was detected by valgrind). They would be extremely useful when debugging a custom allocator. Thanks @geoffmcl !

balthisar commented 6 years ago

@geoffmcl, the sprtf.h/c references are still in the CMakeLists.txt file, and I don't need them to be there (they just cause a notice for me when I build). I don't want to remove them until you have a chance to check them on Windows. I added them myself earlier if this request, but with build systems, better safe than sorry.

geoffmcl commented 6 years ago

@balthisar well they were always there, just under an if (MSVC)...

As discussed elsewhere it is not really possible to switch things according to the MSVC build configuration, Debug, Release, etc, at the cmake level... and I only recently got my auto-special-debug back working as I had, and would like to keep that, if possible...

What is the notice you are seeing?

The sprtf.c has no code if ENABLE_DEBUG_LOG is not on. I know some compilers warn when there is no code in a module... easily fixed by putting some small thing outside the macro... or is it something else...

Maybe the original macro could be changed to if (MSVC OR ENABLE_DEBUG_LOG), or something...

What are you suggesting? Thanks...

balthisar commented 6 years ago

What are you suggesting? Thanks...

Nothing, at this point. Given that everything works on multiple platforms, and it looks like everyone finds the behavior acceptable, I'd vote for closing this.

geoffmcl commented 6 years ago

@balthisar although still thinking about adding an if (MSVC) option for DISABLE_DEBUG_LOG have decided this is quite minor, so will forget it for now...

Accordingly, agree, seems this can be closed...