Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Class / Namespaces to Rename / Move #550

Closed
19 of 33 tasks
ptheywood opened this issue Jun 3, 2021 · 0 comments · Fixed by #1007
Closed
19 of 33 tasks

Class / Namespaces to Rename / Move #550

ptheywood opened this issue Jun 3, 2021 · 0 comments · Fixed by #1007
Assignees

Comments

@ptheywood
Copy link
Member

ptheywood commented Jun 3, 2021

Making an issue to keep track of class / namespaces / files I come across during namespacing that I feel should be renamed.

This list will evolve over time, and at some point I'll bring things up for dicussion.

  • FGPU_SA / exception/FGPUStaticAssert.h -> flamegpu::util::StaticAssert
    • static_assert is a reserved word, so might still want renaming?
  • Move additional things into detail:
    • util::StaticAssert - needed in headers for template reasons, but not intended to be user-facing.
    • curve/cuRVE - and/or probably also needs its own namespace, so flamegpu::curve::Curve and flamegpu::curve::detail, or flamegpu::detail::curve, although it also has some namespaced internal stuff, though it should be safe to place that in flamgepu::detail::curve` in general.
    • model/*Data - this is not inded for users to use, but they kind of have to? Unsure.
    • util A significant amount of util is not intended to be user-facing, could move to flamegpu::detail::util / flamegpu::util::detail. NVTX is semi-user facing however.
  • Additional sub namespaces
    • flamegpu::io - the IO directory makes sense as a group of things. It will grow over time as more formats are supported, so using an io namespace may make sense (and match disk)
  • FGPUException
    • Rename to FLAMEGPUEException? or just flamegpu::Exception? Not sure how this will translate to python, or if using the generic name Exception is a good idea, even inside a namespace.
  • flamegpu_internal -> replace with flamegpu::detail
  • flamegpu/runtime/flamegpu_api.h only includes 3 other header filles. It is superflous. Users should include flamegpu/flamegpu.h, as should any tests.
  • Should include/flamegpu/runtime/messaging(_device).h actually be in include/flamegpu/runtime/messaging/?
  • io/* - CapitolCase for class/filenames?
  • curve_internal -> curve::detail
  • ModelVis:model is actually a reference to a CUDASimulation. ModelVis::simulatiion or SimulationVis?
  • Simualtion/CUDASimulation. Simulation or Simulator? Doesnt' really matter grand scheme.
  • FLAMEGPU_Visualisation - Now we have name spaces this is just flamegpu::visualiser::Visualisation?
  • flamegpu::FLAME_GPU_AGENT_STATUS -> flamegpu::AGENT_STATUS. Its in the namespace now so redundant? This probably applies to many things.
  • Lots of variables in the examples could have more meaningful / accurate / consistent names.
    • cuda_model -> cudaSimulation
  • Stock::Model namespaces start with uppercase?
  • Exception classes should probably either go in an exception namespace, and /or contain Exception in their name? Mostly for the docs?
  • tests/test_cases to tests/cxx or similar, to avoid potential confusion with tests/swig/python. Alternatively move the tests/swig/python into swig/tests/python or swig/python/tests?
  • Why does ModelData.h require MsgBruteForce specifically? Not MsgNone? or some other base Msg class?
    • MsgNone doesn't have the Data member struct, which is what ModelData needs as the base class it builds a vector of.
  • a lot of friend relationships in headers which seem less than idea. I.e. why does EnvironmentDescription need CUDAEnsemble::simulate() as a friend? Seems like this will be leading to a lot of includes that aren't really required and may be negatively impacting compile time
  • CUDASimulation.h includes flamegpu/visualiser/ModelVis.h even if VISUALISATION is not enabled.
  • flamegpu/flamegpu_api.h Should this not just be flamegpu/flamegpu.h following convention, or even .cuh given it will include .cuh files eventually (with no valid option to wrap with stuff that prevents cuda builds. Breaking change will occur regardless.
  • given this is flamegpu2, should we use the namespace flamegpu2 rather than flamegpu? The same would then apply for lots of internal use. (I don't think this is required tbh, but worth thinking about).
  • flamegpu.h doesn't protect vis headers via the macro. This seems like a poor choice. Alternatively, the vis api could still be available but noop if vis is not enabled, More expensive but migth clean up main.cu files?
  • RunPlanVec to RunPlanVector
  • RunLog doesnt' have it's own header.
  • Various sub classes in messages, such as CUDAModelHandler. As these are not part of the API they could be moved into detail?
  • no python tests check id_t
  • no python tests for device vector - Separate issue exists already DeviceAgentVector Py Tests #557
  • AgentVector_Agent.h contains 2 classes, AgentVector_Agent and AgentVector_CAgent. Seems to just have been split out to shorted the file, but it's causing issues / faff with swig.
    • This is difficult to split out due to dependency loop(s)
  • enum FLAME_GPU_CONDITION_RESULT -> flamegpu::CONDITION_RESULT
@ptheywood ptheywood self-assigned this Jun 28, 2021
@ptheywood ptheywood mentioned this issue Jul 7, 2021
27 tasks
This was referenced Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant