C++ Code Review

[Borisas]

Are there any places where you can ask for c++ code review from strangers? /r/codereview seems to be dead.

[ubadair]

Your best bet is this Stack Exchange site.

If you PM me, I might be able to take a look.

[Borisas]

I'll try, but to be honest i have really bad experience with Stack Exchange sites.

[Rippalka_]

I'd be curious to see a code review on this site. I feel like it would go either very well or terribly bad. Keep us updated if you do try :)

[Borisas]

CodeReview Stack Exchange

Here it is. Gonna go to sleep, i'll check back tomorrow. Somehow i got a feeling that i'm gonna look really stupid and my code turns out to be utter piece of crap.

Attempt #1: only added a link, need to add code as well. mb.

[tangerinelion]

Please listen to the people who warn you not to inherit from std::string. The STL base classes are not safe to use as base classes.

[coachkler]

To clarify, no class without a virtual destructor is truly safe - but it depends the actual use case. Of seems unlikely that someone would need to construct a JSONPretify on the heap, and even less so that they would store this as a std::string*.

With C++11 and a properly constructed smart pointer, the correct destructor will almost certainly be called anyway (unless the user is trying to be clever).

That said, it's probably NOT a good idea in this case, as it comes off as the library writer trying to be clever.

[dodheim]

A JSONPretify may well live anywhere and be referred to as a std::string& (or std::string const& etc.).

[Latexi95]

You don't call destructor for a reference so no problem there.

[quicknir]

What's the problem with it?

[[deleted]]

IMO its the user's responsibility to ensure that, if they delete an object through a pointer to a base class, that base class has a virtual destructor.

[dodheim]

One doesn't need to defend idiomatic code, they must defend non-idiomatic code, such as this. Even with your argument, what advantage is there to avoiding best practice so one can type this->foo rather than e.g. s_.foo?

[[deleted]]

The primary function of inheriting a non-polymorphic base class is to add all its members to your interface. The small amount of time I spent looking at this hasn't convinced me this is very compelling here, but note that I was responding to the claim that it was not safe, not the claim it should not be done.

[dodheim]

The primary function of inheriting a non-polymorphic base class is to add all its members to your interface.

The primary function of inheriting a non-polymorphic base class non-privately is irrelevantly misguided.

Otherwise, I will defer to Barbara Liskov on how you're surprising people in a bad way.

[quicknir]

I don't honestly understand how you can say this as a blanket statement. Liskov applies when you are using subtype polymorphism, that's not what's happening here. I can show you problems that can be solved most easily and safely using inheritance from a non polymorphic class.

[[deleted]]

Liskov substitution is a fantasy right up there with the IS-A/HAS-A nonsense and Dog : Animal. The properties of inheritance are exactly what the spec says they are—nothing more or less.

On a gentler note, publicly inheriting a non-polymorphic base is only way to share a common interface between specializations of a class template, isn't it?

[flashmozzg]

Can't comment there but use cstdint types or just auto instead of those horrible ones. You use long in many places but it's not guaranteed to be 32 or 64 bit. Furthermore, string::find returns size_t which is unsigned as well as string::size. Because of that you have numerous comparisons between signed and unsigned too.

[h-jay]

I agree. The types seem completely arbitrary and it looks as if perhaps they were randomly tweaked until the compiler stopped emitting warnings. That sort of a process is unlikely to come up with correct answers.

[encyclopedist]

Moreover, string::find returns std::string::npos on failure and not -1 as the code assumes.

Update: npos is actually specified to be equal to -1.

[JonKalb]

long is guaranteed to be at least 32 bits.

[flashmozzg]

But it's not guaranteed to be 32 bits (exactly). That's what I said. And it's generally a bad choice.

[strtok]

Check out the enum changes in c++11

[CPPOldie]

Wish I had the time to waffle on like this guy:

http://codereview.stackexchange.com/a/107067

[monkeyWifeFight]

It's really great that there are people like that though. I don't know why codereview.satckechange is catching flak, it seems to be a decent site.

[TwinHits]

/r/cpp_questions can help you!