Register a SA Forums Account here!
JOINING THE SA FORUMS WILL REMOVE THIS BIG AD, THE ANNOYING UNDERLINED ADS, AND STUPID INTERSTITIAL ADS!!!

You can: log in, read the tech support FAQ, or request your lost password. This dumb message (and those ads) will appear on every screen until you register! Get rid of this crap by registering your own SA Forums Account and joining roughly 150,000 Goons, for the one-time price of $9.95! We charge money because it costs us money per month for bills, and since we don't believe in showing ads to our users, we try to make the money back through forum registrations.
 
  • Post
  • Reply
Lexical Unit
Sep 16, 2003

Saw this today:
code:
	void Foo(string src)
	{
//  //			printf ("debung bullshit blah blah....\n",
//				stuff, things);
//	old_code = left::behind;
		Bar b;
		b.name = src;
		map<string, Bar>::iterator it=Bars.find(src);
		if(it==Bars.end())
		//if(it!=Bars.end())
		  Bars.insert(pair<string, Bar>(src, b));
		else (*it).second=b;
	DEBUG_PRINT("Poo pants!");
	}
Mixing tabs and spaces for code indention, check. Commented out lines twice, for good measure, check. Old as hell left over code all over the place but commented out, check. Besides all that, why -- when this would have sufficed:
code:
void Foo(string src)
{
	Bars[src] = Bar (src);
}

Adbot
ADBOT LOVES YOU

Lexical Unit
Sep 16, 2003

Victor posted:

Clearly, return is a function. :psyduck:
Sometimes I'll do this like if I'm returning a the boolean value of an expression or using ?: or something, just to bring attention to it. So like return (foo ? "foo" : "not foo"); or return (a && (b || c));. I don't know, it also looks somehow cleaner to my mind. V:shobon:V

Lexical Unit
Sep 16, 2003

Nada, I agree there.

Lexical Unit
Sep 16, 2003

So we use middleware to pass serialized objects around a network of machines. All code is c++. We have to interface with other people's code over this middleware. They define the objects that get sent back and forth over the middleware. Here's a "snippet" (not really, but to give you an idea of the horror) of their headers:
code:
#define int_32 long
#define int_64 long long

typedef struct {
	int_32 a;
	char b[512];
	double c;
	int_64 d;
	float f;
} foo;
This code has to be complied on a mix of 64 and 32 bit machines and there's no way to tell if a message any given machine gets was sent from a 32 bit machine or a 64 bit machine. Also, little vs big endianness comes into play as the endianness of the serialized object doesn't necessarily have to match the endianness of either the sending or receiving machines.

...

:argh:

Lexical Unit
Sep 16, 2003

Ok, but, the software is called "middleware." And, conceptually, it stands between two computers. Also, the messaging is platform agnostic. What you put in is what you get out, no matter what platform you're on. So if a 64 bit machine puts in a 20 bit object, a 32 bit machine will receive a 20 bit object, even if the source for that object compiles to a 16 bit object under 32 bit compilation. Hence the horror.

Lexical Unit
Sep 16, 2003

fansipans posted:

...
...
...
...

...

Are you making fun of me? It was a pause for dramatic effect! :smith:


MrMoo posted:

Technically that middleware is tied to the sending system.
Ah I see what you mean now. And yes, that would be an accurate assessment.

Lexical Unit
Sep 16, 2003

JoeNotCharles posted:

Oh, god, that reminds me of one of my pet peeves: ".lenght()"
I can one-up that. A struct with longitude, latitude, and longititude. No longititude is not different that longitude, it is just there because somehow longititude got into a spec sheet and it was easier to add to the spec than change it. Old code expects longititude to be populated, new code expects longitude to be populated. Good night, and good luck.

Lexical Unit
Sep 16, 2003

Ah, the classics have a beauty to them that is unparalleled.

Lexical Unit
Sep 16, 2003

I started working on a new project yesterday so I was reading some of the code trying to get a feel for how things are organized. I noticed two files called DirectoryFileObjects.h and two files called DirectoryFileObjects.cc. I diffed each pair to see what was up, they were the same. So I went into the revision tool and what I see is that one of them is an original and the other is just a copy that is kept in sync with changes made to the original.

Like, the revision comments on one of the pair of files is something like:
- added blah support

And then the revision comments on the other file (for the same change) is:
- synced with master

Why? Because the developer needed some of the functions defined in DirectoryFileObjects.h, but he needed them somewhere else than where the original DirectoryFileObjects.h exists. :psyduck:


And that's not mentioning the 2345 line function in DirectoryFileObjects.cc whose last 14 lines are nothing but closing braces stepping down 14 levels of indentation. :bravo:

Lexical Unit
Sep 16, 2003

I was trying to fix a compile issue due to an upgrade to a newer version of a vendor provided library. The code was C++. The writer of this library was sitting behind me and I was at the keyboard. I opened up one of my files that had a compile error and was fixing it when the guy behind me says:

Guy: What's that syntax there?
Me: Eh?
Guy puts finger on screen to point at a &
Guy: That.
Me: Oh, I don't want to copy that object so I passed it by reference.
Guy: What's that?
Me: Um. It's a reference.
Guy: I don't get it. Is that like a pointer or something? I usually use that to mean address-of.
Me: Well... it's kinda like a pointer... but it's a reference.
Guy: I don't know about that, I'd take it out to be safe.
Me: Uh huh. Well. I'll take my chances.

For fucks sake this guy wrote nearly 300 kloc of C++ and he doesn't know what a reference is? gently caress me.

Lexical Unit
Sep 16, 2003

The library is the C++ implementation for a CORBA client using TAO. It is most assuredly C++ in every conceivable way. The guy billed himself as "The C++ Guy," not "The C Guy Who Just Happens To Use A C++ Compiler." His job description could have literally been "The Guy Who Implemented This Library In C++."

Lexical Unit
Sep 16, 2003

Anonymous Name posted:

unclear code
So long as you practice good const correctness, this issue should be made pretty clear from the function signature. And really, if you're using a function, I'd expect you'd be familiar with that much at the very least.
code:
int foo(const object& input, object& output);
Also sometimes it's not just optimization that a reference is used for, but some objects simply don't have value semantics and shouldn't ever be copied. When dealing with some opaque object from some vendor's library, it's best just to be safe and not assume things will just work. It'd be nice if they actually documented their library so I would know exactly how their objects can be used... but that's just a pipe dream.

Lexical Unit
Sep 16, 2003

floWenoL posted:

The person who wrote the code may be completely familiar with the function, but maybe not the person reading the code later.
So what is the person who's reading the code going to get out of reading it if they don't know what any of the functions do? :raise:

Lexical Unit
Sep 16, 2003

I need to test if this number is odd. Think man, think! Hey isn't there something about odd numbers and dividing by two that I learned in kindergarten?

<A couple of hours later, after three of four pages of scratch work>

Oh hey, look at this, whenever I divide an odd number by 2 I get .5 on the end... regex to the rescue!

Lexical Unit
Sep 16, 2003

My co-worker told me today that const-correctness isn't worth the speed hit.

Lexical Unit
Sep 16, 2003

seiken posted:

I've always gotten by fine, is this a horrible thing?
It kinda sucks :(

I mostly design libraries to be used by other people and one of the primary drives for being const-correct for me is common courtesy. If I provide a method bar() const on my object foo, then users of my library know that calling it isn't going to (conceptually) mutate the object.

Going back and trying to const-correctify something that wasn't designed to be const-correct from the start is painful.

Lexical Unit
Sep 16, 2003

Found this code today:
code:
const char *host = getenv ("DATA_HOST");
if (!host) host = "";

const char *path = getenv ("DATA_PATH");
if (!path) path = "";

const char *file = /* got from a drop down box, can possibly be "" */;

char cmd[256];
sprintf (cmd, "rsh %s /bin/rm -rf %s/%s &", host, path, file);

FILE *ptr = popen (cmd, "r");
if (ptr) {
  char buf[256];
  while (fgets (buf, sizeof (buf), ptr) != NULL) {
    (void) fprintf (stderr, "%s", buf);
  }
  int rc = pclose (ptr);
} else {
  perror ("popen");
}

if (0 != rc) {
  char message[256];
  sprintf (message, "unable to delete %s", file);
  fprintf (stderr, "%s", message);
}
Edit: Oh yes, I forgot to mention this code is found in a .cpp file.

Lexical Unit fucked around with this message at 22:57 on Feb 27, 2009

Lexical Unit
Sep 16, 2003

Actually, more than 5 people have touched the file over the course of 5 years and all 5 of these people still work here. In fact, 3 of the 5 can be considered my supervisors.

Lexical Unit
Sep 16, 2003

Good question. I asked my boss the same thing. He shrugged. He is one of the 5.

Edit: The rabbit hole gets deeper. The program comes with a .env that should be sourced before execution. That file looks like this:
code:
export DATA_HOST=${DATA_HOST:-"ERROR: undefined"}
export DATA_PATH=${DATA_PATH:-"ERROR: undefined"}
Problem solved! :downs:

Lexical Unit fucked around with this message at 23:55 on Feb 27, 2009

Lexical Unit
Sep 16, 2003

mr_jim posted:

I really hope that the program is running under an unprivileged account, but that might be expecting too much.
Yeah no. It runs as root.

Lexical Unit
Sep 16, 2003

Avenging Dentist posted:

If it weren't for the fact that you're setting to NULL twice, this is completely acceptable and is in fact a recommended thing to do. (Though putting the error handling in the catch statement would be even better.)
Wouldn't LocalPlace* plp = new (std::nothrow) LocalPlace; be recommended over that code though? Seeing as all they seem to want to do is to set the pointer to NULL if new fails... Also, couldn't the catch (...) be masking any exceptions that LocalPlace's constructor might throw?

Lexical Unit
Sep 16, 2003

Seth Turtle posted:

This is the proper layout of a programmer's two screens...

Not quite.

1. Your development machine has its screen saver up. (Who cares what windows are up on that thing?)

2. Your personal machine to your left has SA forums up with youtube hidden underneath that window in case your boss walks in.

Lexical Unit
Sep 16, 2003

C++:

Exhibit 1
code:
string foo = returns_a_string ();
if (!strcmp (foo.c_str (), "test")) { ...
// foo used nowhere else in the program
Exhibit 2
code:
struct bar {
	int uid;
	/* megaobject */
};
map<int, bar> m; // global

void handle_event_a(some data)
{
	while(/* we still have some data to consume */)
	{
		bar new_bar;
		// extract a bar from the remaining data, store it in new_bar
		m[new_bar.uid] = new_bar;
	}
}

// at no point in the program is m used like a map
// the only methods ever used on m are operator[](), begin(), and end().
// operator[]() is never called on the map besides to insert/update data, seen above.
// here is where begin() and end() are used:
void handle_event_b()
{
	vector<bar> bars;
	map<int, bar>::iterator i;
	for (i = m.begin (); i != m.end (); i++)
	{
		bars.push_back (i->second);
	}
	do_stuff (bars);
}

void do_stuff(vector<bar> bars)
{
	// simply iterates over bars to do stuff
}
Exhibit 3
code:
int pants_size = 0; // global
int pants_type = 0; // global

void handle_pants_event(int size, int type)
{
	if (pants_size == 0 && pants_type == 0)
	{
		pants_type = type;
		pants_size = size;
	}
	else if (pants_size != size || pants_type != type)
	{
		pants_type = type;
		pants_size = size;
	}
}
Exhibit 4
code:
void handle_input(void* data, unsigned long size)
{
	object o = object::parse (data, size); // allocates and copies memory
	
	switch (o.type)
	{
		case 1: foo (data, size); break;
		case 2: bar (data, size); break;
		case 3: baz (data, size); break;
	}
}

void foo(void* data, unsigned long size)
{
	object o = object::parse (data, size);
	if (o.type != 1) return;
	// use object o
}

void bar(void* data, unsigned long size)
{
	object o = object::parse (data, size);
	if (o.type != 2) return;
	// use object o
}

void baz(void* data, unsigned long size)
{
	object o = object::parse (data, size);
	if (o.type != 3) return;
	// use object o
}
:psyduck:

Lexical Unit fucked around with this message at 21:57 on Jun 2, 2009

Lexical Unit
Sep 16, 2003

In the real code there's more like 20 lines in each branch... they are exactly the same. For a good long second I didn't realize what I was looking at because I couldn't believe what I saw.

Lexical Unit
Sep 16, 2003

Zombywuf posted:

Well, that code will order the incoming events by uid...
Yeah, except that ordering these objects by their uid provides no utility what-so-ever. He's essentially using a map as a set. And then he copies the objects into a vector for no reason because he could just iterate over the global map/set if he wanted. And just to top it off he sends the newly created vector to another function by value. And all that function does is iterate over all the objects... so why create the vector in the first place? It boggles the mind.

I didn't show it but later he takes that same vector, takes the reference of it to get a pointer to it, and passes around that pointer to other functions. If you look in the revision history you can see that in the past he was just passing around the vector by value until finally the program ground to a halt and he changed to passing around pointers to the vector to speed the program up.

Lexical Unit
Sep 16, 2003

Found this in a Perl script today
code:
if (false) { ... }
Of course there was no use::strict in the file.

Lexical Unit
Sep 16, 2003

Yeah because it wouldn't run if it had it :v:

Edit: I see what you mean: s/::/ /

Lexical Unit fucked around with this message at 00:59 on Jun 12, 2009

Lexical Unit
Sep 16, 2003

That was surely their intention, but w/o use strict the bare text false becomes a string literal which evaluates to true :banjo:

Lexical Unit
Sep 16, 2003

This code is now my favorite code:
code:
template<class T>
T ByteSwap(T x)
{
	T r;
	int n = sizeof (T);
	if (n > 8)
		std::cerr << "this is probably an error" << std::endl;
	unsigned char* p = (unsigned char*)&x;
	for (int i = 0; i < n; ++i)
		((unsigned char*)&r)[i] = p[n - 1 - i];
	return r;
}

template<class T>
void swap_data(T* data, int n)
{
	int i;
	switch (sizeof (T))
	{
		case 1: break;
		
		case 2:
			for (i = 0; i < n >> 1; i++)
				*(unsigned short*)(data + i) =
					bswap_16 (*(unsigned short*)(data + i));
			break;
			
		case 4:
			for (i = 0; i < n >> 2; i++)
				*(unsigned*)(data + i) =
					 bswap_32 (*(unsigned*)(data + i));
			break;
			
		case 8:
			for (i = 0; i < n >> 3; i++)
				*(unsigned long long*)(data + i) =
					bswap_64 (*(unsigned long long*)(data + i));
			break;
			
		default:
			for (i = 0; i < n / sizeof (T); i++)
				data[i] = ByteSwap (data[i]);
			break;
	}
}

Lexical Unit
Sep 16, 2003

Painless posted:

among other crimes
In case y'all were curious, here are the lines of code that lead me to inspect the byteswap header file in the first place,
code:
vector<complex<float> > payload;
b.get_payload (payload);
// Do we need to byteswap here??
for (int i = 0; i < payload.size (); i++)
  ByteSwap (payload[i]); // why doesn't this work?!?!
:toot: Yes, those comments were checked in.

Lexical Unit
Sep 16, 2003

gibbed posted:

This is nitpicking and platform specific, although the cerr thing is stupid, yes. :colbert:
I would like to see this.

It seemed mostly OK to me, although I wouldn't make it a template.

I'm not sure about Painless's point about "creating a T instance makes the byteswap a lot less generic." The function is already too generic because it accepts things like struct foo { int i; float f; };, std::complex<float>, int a[10], ...

Normally yes you might want to avoid using a default constructor unless necessary because there's no reason to enforce the type has a default constructor if you don't need to. But in this case I don't think ByteSwap() should work at all on non-integral types, which are all default-constructible (obviously).


Printing an error message is unacceptable as well. There's code that attempts to call ByteSwap() on long doubles, which can exceed (and usually do) 8 bytes in size. It's evident from the history of this file that it's intended to be the end-all to byteswaping for a wide swath of our codebase. That it's intended to "just work" on floating point types. It does not.


As for the runtime switching, I re-coded it as a compile-time switch by just using templates and am seeing a 4x to 16x speed improvement.

There are other issues, namely:

Nitpicky, but: "ByteSwap() - CamelCase" vs "swap_data() - lowercase_underscore"; just pick one.

Why should a for loop calling ByteSwap() be slower than a single call to swap_data()? It shouldn't, and it doesn't have to be. Testing has shown that swap_data() is up to 10x faster than a loop over calls to ByteSwap(). The code is loving lazy.

As I already said, the code is too generic. I actually had the guy who wrote the code in my office the other day and he was saying things like, "well it's a template so you can't construct what types it accepts at all! A template accepts everything, you know, that's why it's called a template." A simple application of traits types and the error message isn't necessary and ByteSwap() only compiles when it's being used correctly.

Another thing, did you notice the signature of swap_data()? void swap_data(T* data, int n). What isn't immediately obvious is that n here is supposed to be the size of T * number of Ts. Why not avoid the issue entirely with a better signature? Or at least make the parameter size_t size or something for fucks sake.
code:
// ugly:
vector<int> ints = get_some_ints ();
swap_data (&ints.front (), ints.size () * sizeof (ints.front ()));

// why not this instead:
vector<int> ints = get_some_ints ();
swap_data (ints.begin (), ints.end ());

// and this too:
size_t n = get_n ();
int* ints = get_n_ints (n);
swap_data (ints, ints + n);

sex offendin Link posted:

There is no generalized way to byte-swap IEEE 754 numbers anywhere, they are for all intents and purposes non-portable.
What about putting them in a union with an unsigned integer type, byteswapping the integer and returning the integer? The integer can go over whatever I/O it needs to and be byteswapped back into a floating point type on the other end.

I'm honestly asking, wouldn't this work?

Lexical Unit fucked around with this message at 04:06 on Jul 8, 2009

Lexical Unit
Sep 16, 2003

So if you leveraged char* to do the swapping rather than a union hack, and for floating point types you returned an accommodating integer type rather than storing a byteswapped floating point value back into a floating point type, would you be reasonably safe then?

Probably due to lovely websites found through google:

quote:

the compiler very probably will return the double return value in an FPU register, and if not then, at some point the value may be loaded into an FPU register. This means the swapped result is loaded into an FP register, which then means the FP unit will take a look at the number, and then the fun begins.

when you swap,You move some of the mantissa data into the sign/exponent slot and the sign/exponent data becomes part of a mantissa. And guess what? Not all values of sign/exponent/mantissa are valid floating point numbers according to IEEE 754.

When a floating point unit is faced with an invalid value, it may actually change the value, or worse, throw an exception.
\/\/

Lexical Unit fucked around with this message at 04:29 on Jul 8, 2009

Lexical Unit
Sep 16, 2003

Nevermind. I really need some sleep. What you say makes perfect sense.

Lexical Unit fucked around with this message at 05:50 on Jul 8, 2009

Lexical Unit
Sep 16, 2003

code:
ImageMap::ImageMap()
{
    memset (this, 0, sizeof (*this));
}
C++, non-POD data members.

Edit: wow http://bytes.com/groups/cpp/621474-setting-every-bit-all-members-class-0-a

This kind of shortcut had never even occurred to me...

Lexical Unit fucked around with this message at 21:15 on Jul 23, 2009

Lexical Unit
Sep 16, 2003

Haha, same file:
code:
#define SIN(x) (sin((x)*M_PI/180))
Note that SIN() is only ever called once in the file, and it's never #undefed.

Lexical Unit
Sep 16, 2003

pokeyman posted:

code:
#define I 1
...
#define LVI 56
...
#define DCCXXXV 735
...
Everyone knows macros are horrible, so use mpl::string instead!
code:
template<char> struct numeral_c { static const long value = 0; };
template<> struct numeral_c<'M'> { static const long value = 1000; };
template<> struct numeral_c<'D'> { static const long value = 500; };
template<> struct numeral_c<'C'> { static const long value = 100; };
template<> struct numeral_c<'L'> { static const long value = 50; };
template<> struct numeral_c<'X'> { static const long value = 10; };
template<> struct numeral_c<'V'> { static const long value = 5; };
template<> struct numeral_c<'I'> { static const long value = 1; };
template<> struct numeral_c<'i'> { static const long value = 1000; };
template<> struct numeral_c<'v'> { static const long value = 5000; };
template<> struct numeral_c<'x'> { static const long value = 10000; };
template<> struct numeral_c<'l'> { static const long value = 50000; };
template<> struct numeral_c<'c'> { static const long value = 100000; };
template<> struct numeral_c<'d'> { static const long value = 500000; };
template<> struct numeral_c<'m'> { static const long value = 1000000; };

template<class Total, class Iter>
struct numeral_fop
{
	typedef typename boost::mpl::deref<Iter>::type element;
	typedef typename boost::mpl::deref<typename boost::mpl::next<Iter>::type>::type next_element;
	typedef typename boost::mpl::if_<
		boost::mpl::less<
			typename boost::mpl::long_<numeral_c<element::value>::value>::type
			, typename boost::mpl::long_<numeral_c<next_element::value>::value>::type
		>
		, boost::mpl::long_<Total::value - numeral_c<element::value>::value>
		, boost::mpl::long_<Total::value + numeral_c<element::value>::value>
	>::type type;
};

template<class String> struct numeral :
boost::mpl::iter_fold<
	String
	, boost::mpl::long_<0>
	, numeral_fop<boost::mpl::deref<boost::mpl::_1>, boost::mpl::_2>
>::type
{ };

typedef numeral<boost::mpl::string<'I'>::type>::type I;
typedef numeral<boost::mpl::string<'LVI'>::type>::type LVI;
typedef numeral<boost::mpl::string<'DCCX','XXV'>::type>::type DCCXXXV;
Considering this code is my first foray into boost::mpl, I figure this thread is a good place for it.

Lexical Unit
Sep 16, 2003

Tell me about it! That stupid code accepts things like IIII and IIV. Madness.

Lexical Unit
Sep 16, 2003

code:
class a_context { /* ... */ };
class b_context { /* ... */ };
class c_context { /* ... */ };

template<class Context>
class _foo
{
  // ...
};

#define a_foo _foo<a_context>
#define b_foo _foo<b_context>
#define c_foo _foo<c_context>

Lexical Unit
Sep 16, 2003

code:
void pants_event(char* data, size_t size, long id)
{
	switch (id)
	{
		case 1:
			handle_pants (data, size, id);
			break;
		case 2:
			handle_pants (data, size, id);
			break;
		case 3:
			handle_pants (data, size, id);
			break;
		case 4:
			handle_pants (data, size, id);
			break;
	}
}

void handle_pants(char* data, size_t size, long id)
{
	switch (id)
	{
		case 1:
			{
				// do stuff
			}
			break;
		case 2:
			{
				// do stuff
			}
			break;
		case 3:
			{
				// do stuff
			}
			break;
		case 4:
			{
				// do stuff
			}
			break;
	}
}

Adbot
ADBOT LOVES YOU

Lexical Unit
Sep 16, 2003

Oh the things I could draw if posts were whiteboards...

  • 1
  • 2
  • 3
  • 4
  • 5
  • Post
  • Reply