Comment by msim | Hacker News Reader

Comment by msim | Hacker News Reader

Parent Full thread

It's code like this that gives C++ a bad reputation. It's not modern in any sense. Compiling it with my default warning level in clang gives 482 warnings! Here's a summary:

  warning: cast from '...' to '...' increases required alignment from 1 to X [-Wcast-align]
  warning: declaration shadows a field of '...' [-Wshadow]
  warning: declaration shadows a local variable [-Wshadow]
  warning: implicit conversion changes signedness: '...' to '...' [-Wsign-conversion]
  warning: implicit conversion loses integer precision: '...' to '...' [-Wconversion]
  warning: implicit conversion loses integer precision: '...' to '...' [-Wshorten-64-to-32]
  warning: macro name is a reserved identifier [-Wreserved-id-macro]
  warning: no previous prototype for function '...' [-Wmissing-prototypes]
  warning: operand of ? changes signedness: 'int' to 'char' [-Wsign-conversion]
  warning: unused parameter '...' [-Wunused-parameter]
  warning: use of old-style cast [-Wold-style-cast]

Use with caution!

So you enabled pedantic warning level, and you got a bunch of pedantic nonsense warnings.

There is a reason for these not to be enabled by default.

* Unused parameter -> rly? Who gives a damn? * Use of old style cast -> Well I'm old style, get over it. * No previous prototype declaration -> Again, I do this if I want to. * Shadows field -> who cares? No me. * Cast increases required alignment -> Well, obviously the perf cost is not an issue here. * Etc, etc, etc

These are pedantic warnings. However, this is an open source prject and you are free to send me PR's whenever you want.

Maybe using a `reinterpret_cast` would actually have better performance. A C-style cast tries to do a bunch of casts in order, starting with a static and ending with reinterpret (see http://anteru.net/blog/2007/12/18/200/).

There's no excuse for using c-style casting in C++ considering the depth of the different casts we as developers have at our disposal.

Additionally, I just looked at the code, and is there any reason it's all stuffed into a single header and source file? I don't know if I'm just being naive, but isn't this a slightly bad design? There seem to be a lot of different data structures that could easily be broken out and make it a bit easier to follow the flow of the project.

It's funny that you don't mention the warnings that can be security vulnerabilities, like integer precision/signedness.

The code screams security vulnerability and I'm not only talking about the warnings.

Yep, I seem to recall there was one in MySQL a while back that just involved a simple int->char conversion where the overflow would trigger a match due to a random seed that was involved in the password path.

I'm a big fan of Wall Werror with pragma for specific sections where you must work around the warnings. Does a great job of catching issues with contributions.

"Use of old style cast -> Well I'm old style, get over it."

"By using Linux I haven't been limited by the lacking Microsoft C++ compilers only supporting a fraction of the language, but instead been able to use the very latest features and tools."

Why do I care if it's using an "old-style cast" (I presume something like `(float)var` instead of `float(var)`, replacing a reserved identifier with a macro, or contains implicit casts (like `1.0 + 1` instead of `1.0 + float(1)`)?

Don't C++ compilers give lots of unimportant warnings?

Yes, my friend. This guy is trying to roast the library based on bullshit warnings that normal projects (Node.js as one example) explicitly disable. So if you want to make a case out of this, you will have to report this "issue" to Node.js developers also, because they also have a lot of warnings ignored by turning them of, simply because they are pedantic.

Reply on news.ycombinator.com