Next fix vsnprintf by dschwoerer · Pull Request #365 · boutproject/BOUT-dev

dschwoerer · 2016-11-03T19:00:25Z

Inlike the PR #319 (the PR for the master branch) this should also work for the derived exceptions.

Safe version, which guards against buffer overruns by specifying the length of the buffer into which text is being printed. This was used in both Output and BoutException classes to provide C printf-style output.

Missed a couple of instances in BoutException derived classes.

Both Output and BoutException now use a static const int BUFFER_LEN to control the length of the buffer used for C style output.

Conflicts: src/fileio/datafile.cxx

ZedThree

I think only the ShiftOutput thing definitely needs to be fixed, but the dummy argument is a little concerning.

Also, I'm not sure about the name. Maybe bout_vsnprintf, or something, instead?

ZedThree · 2016-11-14T15:17:16Z

src/fileio/datafile.cxx

 #include "formatfactory.hxx"

-Datafile::Datafile(Options *opt) : parallel(false), flush(true), guards(true), floats(false), openclose(true), enabled(true), shiftOutput(false), file(NULL) {
+Datafile::Datafile(Options *opt) : parallel(false), flush(true), guards(true), floats(false), openclose(true), enabled(true), file(NULL) {


This removes shiftOutput(false) from the initializer list. I assume this is accidental?

yes, thanks

ZedThree · 2016-11-14T15:24:18Z

include/utils.hxx

+/// reallocated.
+/// len: the length of said buffer. May be changed, mussn't be const.
+/// fmt: the const char * descriping the format.
+/// va: A dummy argument. will


Is the dummy argument needed? It looks confusing when the macro is called.

you are right, removed it 👍

ZedThree · 2016-11-14T15:28:49Z

src/field/field.cxx

  }

  throw BoutException("Error in field: %s", err_buffer);
+  delete[] err_buffer;


This will never be hit, and presumably leads to a memory leak if the exception isn't caught?
I'm not really sure a) if this is a problem and b) if so, how to fix it.

This leads to a memory leak, no matter if caught or not. (not caught however doesn't really matter)
b) use std::string

I added BoutException(std::string) in order to avoid this issue

ZedThree · 2016-11-14T16:02:46Z

Maybe in future it would be possible to replace all this C stuff with nice C++ idioms? This library looks like a really nice replacement for the printf family. It could possibly reduce a lot of the complexity about formatting messages.

dschwoerer · 2016-11-14T17:34:42Z

Does "this library" (fmt?) support traditional formatting like %-10.3g?
This is the main reason why I don't like the C++ way - it is to bloated for number printing ...

ZedThree · 2016-11-14T17:36:17Z

From what I can tell, it supports not only traditional printf style printing like that, but also python style:

 fmt::format("The answer is {}", 42);

dschwoerer · 2016-11-14T17:38:22Z

Only the python format style - python also supports c-printf like statements ...( as the one above, left centered, at least 10 chars, and 3 digits after the point)

Update: sorry, I missed 'not only'.
From the description I didn't saw a single example for that, so I am not sure how good the floating point number support is ...

ZedThree · 2016-11-14T17:50:23Z

Yep, look at fmt::printf, fmt::fprintf and fmt::sprintf:

 fmt::printf("The answer is %d\n", 42);

Also, see the SO answer here. Looks like we can do all this in pure C++, but still use printf format strings. Everyone's happy! :)

I'll have a bash at it tomorrow.

ZedThree · 2016-11-28T16:34:49Z

Is this still needed for v4.0.0-RC? It looks quite similar to what is in there now - I think there was also a PR against master?

Also, is it possible that this functionality could be done using C++11 features instead of macros, e.g. std::snprintf? See http://stackoverflow.com/a/26221725/2043465

dschwoerer · 2016-11-28T16:51:28Z

Using variadic templates requires that all this is defined in header files ...

I cannot see that this is already fixed in v4 RC:
https://github.com/boutproject/BOUT-dev/blob/v4.0.0-RC/src/sys/boutexception.cxx#L80

bendudson · 2016-11-28T17:06:59Z

This would be better than what's there now, but using type safe templates might be better

…

On 28 November 2016 at 16:51, dschwoerer ***@***.***> wrote: Using variadic templates requires that all this is defined in header files ... I cannot see that this is already fixed in v4 RC: https://github.com/boutproject/BOUT-dev/blob/v4. 0.0-RC/src/sys/boutexception.cxx#L80 — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#365 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AANYYcBU6wdj4PTd1OuHes-d3n81_6EWks5rCwaQgaJpZM4KoyYc> .

bendudson · 2016-12-01T13:38:28Z

To avoid best becoming the enemy of better, I vote we merge this for now, and then look at changing to a C++ solution later.
Can you (David) please change the base to v4.0.0-rc?

…nprintf

…x-vxnprintf

dschwoerer · 2016-12-01T15:28:33Z

base changed + conflicts resolved:
should be ready to merge ...

bendudson and others added 8 commits October 13, 2016 17:38

Replace vsprintf with vsnprintf

74b8c03

Safe version, which guards against buffer overruns by specifying the length of the buffer into which text is being printed. This was used in both Output and BoutException classes to provide C printf-style output.

More vsprintf to vsnprintf in BoutException

2d2278f

Missed a couple of instances in BoutException derived classes.

Make consistent BUFFER_LEN constant

26efa2c

Both Output and BoutException now use a static const int BUFFER_LEN to control the length of the buffer used for C style output.

fixing the issue with vsprintf

237cf5d

minor fixes

12a819c

more fixes

2e05f80

Merge remote-tracking branch 'bp/next' into next-fix-vxnprintf

af2f201

Conflicts: src/fileio/datafile.cxx

missing static member added

aeacf65

ZedThree reviewed Nov 14, 2016

View reviewed changes

dschwoerer added 2 commits November 16, 2016 02:26

rename macro + remove dummy argument

a687662

fix memory leak

1b09890

dschwoerer changed the base branch from next to v4.0.0-RC December 1, 2016 15:15

dschwoerer added 2 commits December 1, 2016 15:25

Merge remote-tracking branch 'boutproject/v4.0.0-RC' into next-fix-vx…

3224de8

…nprintf

Merge remote-tracking branch 'origin/next-fix-vxnprintf' into next-fi…

31252c4

…x-vxnprintf

bendudson approved these changes Dec 1, 2016

View reviewed changes

bendudson merged commit af451f3 into boutproject:v4.0.0-RC Dec 1, 2016

dschwoerer deleted the next-fix-vxnprintf branch February 27, 2018 01:47

Conversation

dschwoerer commented Nov 3, 2016

Uh oh!

ZedThree left a comment

Choose a reason for hiding this comment

Uh oh!

ZedThree Nov 14, 2016

Choose a reason for hiding this comment

Uh oh!

dschwoerer Nov 16, 2016

Choose a reason for hiding this comment

Uh oh!

ZedThree Nov 14, 2016

Choose a reason for hiding this comment

Uh oh!

dschwoerer Nov 16, 2016

Choose a reason for hiding this comment

Uh oh!

ZedThree Nov 14, 2016

Choose a reason for hiding this comment

Uh oh!

dschwoerer Nov 14, 2016

Choose a reason for hiding this comment

Uh oh!

dschwoerer Nov 16, 2016

Choose a reason for hiding this comment

Uh oh!

ZedThree commented Nov 14, 2016

Uh oh!

dschwoerer commented Nov 14, 2016

Uh oh!

ZedThree commented Nov 14, 2016

Uh oh!

dschwoerer commented Nov 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZedThree commented Nov 14, 2016

Uh oh!

ZedThree commented Nov 28, 2016

Uh oh!

dschwoerer commented Nov 28, 2016

Uh oh!

bendudson commented Nov 28, 2016 via email

Uh oh!

bendudson commented Dec 1, 2016

Uh oh!

dschwoerer commented Dec 1, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dschwoerer commented Nov 14, 2016 •

edited

Loading