mn416 / QPULib

Language and compiler for the Raspberry Pi GPU
Other
429 stars 64 forks source link

Add namespace `qpulib` to Library files #35

Closed wimrijnders closed 6 years ago

wimrijnders commented 6 years ago

This is grunge work, touching almost every single source file in the project. However, I regard a namespace to be essential for proper integration, so I have persevered.

Even though so many files have been edited, the changes are straightforward and repetitive, so it should be easy to review.

In addition:

Notes

mn416 commented 6 years ago

I would have preferred QPULib to be used as the namespace, to be consistent with the name of the library and the include file. But can live with qpulib.

wimrijnders commented 6 years ago

OK, don't merge yet.

This is just a personal habit of mine, to do namespaces lower case. I just want to check what the overall consensus is on namespace naming. If it's like you say, I'll go with that.


EDIT: Online spot check for naming conventions namespaces

lowerCamel

UpperCamel

lower_case

wimrijnders commented 6 years ago

I've been thinking about why I use lower case for namespaces. I do it because they are then visually distinct from classes.

  // e.g. lower:
  some::name::SomeClass &var = other::name::OtherClass::static_call();

  // versus upper:
  Some::Name::SomeClass &var = Other::Name::OtherClass::staticCall();

I.e. with lower casing it's instantly obvious that some, name and other are namespaces. With Some, Name and Other they might also be class scopes. It makes the code just a bit easier to read.


But I'll let you make the final call. Did I convince you enough with this to use lower? Or do you still prefer upper?

mn416 commented 6 years ago

Aesthetically, I prefer mixed case for multi-word identifiers, so personally I'd go for that.

But I'd rather have your patch than not have it, regardless of case. So I'll merge this in a few days time even if you don't change it.

wimrijnders commented 6 years ago

OK, no problem, really. You have the final say. I'll change it to QPULib. Please wait with merging!

But not right know, I'm not feeling too good (sick). I'll see how I feel in the morning. Right now I'm staying on the couch.

mn416 commented 6 years ago

I'm not feeling too good (sick). I'll see how I feel in the morning. Right now I'm staying on the couch.

Sorry to hear that, I hope you're feeling better soon.

wimrijnders commented 6 years ago

Thanks! Thankfully I got football to watch from the couch.

wimrijnders commented 6 years ago

@mn416 namespace has been renamed to QPULib.

mn416 commented 6 years ago

Hi @wimrijnders,

Thanks for this. Can we remove the Rot3D namespace? I worry that Rot3D is becoming rather complicated, and it's supposed to be one of the super-simple introductory examples. Namespaces are great for integrating different libraries in a large application (which is why I like the QPULib namespace), but I find the extra Rot3D namespace distracting for an introductory example.

After that, I'll merge!

wimrijnders commented 6 years ago

I put the namespace there to simplify the interface. Otherwise, you need to add the QPULib namespace to all relevant parameters in the prototypes. I thought it looks ugly, and this just looked much cleaner. I'll do the code change and then you can decide for yourself. I personally think it's worse.

As to 'super-simple', I'm afraid that I have more changes lined up for Rot3D, in particular being able to specify the kernel and number of QPU's to use on the command line. This would make it easier to run and allow for automation (e.g. profiling). I hope you think that's an OK idea.

wimrijnders commented 6 years ago

OK done. Have a look.

mn416 commented 6 years ago

Why can't we do using namespace QPULib in Rot3DKernels.h?

Edit: Ok, I can see why this might be a bad idea...

wimrijnders commented 6 years ago

Because that would 'pollute' whatever namespace you're including Rot3DKernels.h into, defeating the purpose of namespaces.

As an alternative, you could do the namespace before including, e.g.:

using namespace Rot3D;
#include "Rot3DKernels.h";

...which restricts the 'using namespace' to the current compile unit. Would need some documentation though.

wimrijnders commented 6 years ago

How about this alternative, just came to mind:

Put all type classes of QPULib in a sub-namespace QPULib::Types. Then use that namespace in the include file.

I think that would be safe enough. The major components in QPULib would still be protected by the main namespace.


EDIT: Using nested namespaces will propagate, or course, to code using QPULib. But I believe that the end result will be more robust and usable.

mn416 commented 6 years ago

How about moving Rot3D back to a single-file example, and then creating a new example, say Rot3DLib for demonstrating how to use QPULib to create your own library? You can do whatever you want in Rot3DLib and I will still be happy because good old Rot3D is still there? You might also have Rot3DClient to demonstrate how to use Rot3DLib.

wimrijnders commented 6 years ago

Initial reaction negative because not DRY. Will think about it.

wimrijnders commented 6 years ago

OK, I thought about. The idea has merit.

So there will be the original, simple Rot3D and a more extended one. The original can stay as as and the extended one can evolve.

The only thing that bothers me is the DRY aspect. But I think I can live with that. So the new Rot3D will need a new name. Lib as postfix I don't really like, but I guess good enough.

I propose to put it in a subdirectory Example/Rot3DLib, to keep it separate from the other examples.

wimrijnders commented 6 years ago

@mn416 OK, then. This is the initial version of the split. I'll just forget the DRY bit.

wimrijnders commented 6 years ago

:+1: :tada: :beer: :fireworks: YAY!