When your question is answered use !solved to mark the question as resolved.
Remember to ask specific questions, provide necessary details, and reduce your question to its simplest form. For tips on how to ask a good question use !howto ask.
#Is this good modern C++ (only like 5 files)?
neat horizonBOT
mighty oasis
what I've seen in this feels outdated to me, but I'm not an expert on the topic.
The author is competent, there's no doubt about that. I'm just unsure about some choices the author made. For example, there's goto statements in this. There's also a lot of raw pointers.
chrome leaf
book's not about learning C++ so I dont see a problem
its intended audience is people who are already pretty good at C++ so you could just change the bits u feel like changing
velvet geode
well, upon first glance, it looks like average LLVM code 🤷♂️
it depends on what your goals are
mighty oasis
that probably should be split in 2 goals
outer oar
The author is competent, there's no doubt about that. I'm just unsure about some...
goto is actually not the big problem here
i see pointer arithmetic instead of iterators
velvet geode
you might want to have a look at Tour of C++
outer oar
weird macrofied case-statements in a switch
mighty oasis
`goto` is actually not the big problem here
just a code smell, a hint it might not be modern at all
outer oar
the code is very obtuse
velvet geode
yeah, this is definitely not an example of what good modern C++ looks like. whether it'll teach you LLVM, I have no idea
mighty oasis
yeah I don't have llvm knowledge either, can't say if it's any good
velvet geode
you won't, LLVM is serviceable C++, but it's not great C++.
mighty oasis
something something trying to catch 2 rabbits something somethings
velvet geode
yeah don't ^^
most C++ isn't great
quite honestly, there isn't any I could think of rn.
most code on github is average at best ime
the problem is that any large, established project is generally also an old project.
LLVM was first released in 2003
and it started out in academia
so yeah
grim aurora
Also, another thing is just style. you have your private variables at the top in headers, nobody cares about that, shove it at the bottom of the class.
youre using h for header files yet the files are cpp only, use hpp or smth
also dont be afraid to just use newlines to space out seperate ideas in code
velvet geode
LLVM is fine. but you shouldn't take LLVM code as an example of how to do it.
grim aurora
^^
velvet geode
Also, another thing is just style. you have your private variables at the top in...
fwiw, I put my private parts at the top too. because I do care about them 😛
grim aurora
like this block of code is confusing to read
namespace {
class ToIRVisitor : public ASTVisitor {
Module *M;
IRBuilder<> Builder;
Type *VoidTy;
Type *Int32Ty;
PointerType *PtrTy;
Constant *Int32Zero;
Value *V;
StringMap<Value *> nameMap;
public:
ToIRVisitor(Module *M) : M(M), Builder(M->getContext()) {
VoidTy = Type::getVoidTy(M->getContext());
Int32Ty = Type::getInt32Ty(M->getContext());
PtrTy = PointerType::getUnqual(M->getContext());
Int32Zero = ConstantInt::get(Int32Ty, 0, true);
}
no namespace indendation, massive block of privates, and then what is this ToIRVisitor block? at first glance I have to start reading it like I'm a parser (bad!)
// next line braces are a style thing, I like the logical isolation
namespace
{
// Indentation is good!
class ToIRVisitor : public ASTVisitor
{
public:
// reformatted
ToIRVisitor(Module *M)
: M(M)
, Builder(M->getContext())
{
VoidTy = Type::getVoidTy(M->getContext());
Int32Ty = Type::getInt32Ty(M->getContext());
PtrTy = PointerType::getUnqual(M->getContext());
Int32Zero = ConstantInt::get(Int32Ty, 0, true);
}
private:
Module *M;
IRBuilder<> Builder;
Type *VoidTy;
Type *Int32Ty;
PointerType *PtrTy;
Constant *Int32Zero;
Value *V;
StringMap<Value *> nameMap;
/*...*/
};
} // namespace
this is my reformat
to my eyes this is much easier to read since it actually uses spacing to its advantage to group things into seperate logical chunks
I'm also a stickler for wanting this-> on everything because I get confused easily
but those are just some of my opinions lol
tbh readability is what I look for when glancing at a project, if I don't have time to sit down and understand it algorithmically, I'm going to look at the readability of the code and judge it based on that
if someone doesnt care about readability then why would they care about the code itself?
obv there are exceptions (shaders where you just want the damn thing to work, but theyre short and often dense and filled with constants)
but most of the time, nno
IMO, this is a really good header file
#ifndef SRC_UTIL_FREELIST_INDEX_ALLOCATOR_HPP
#define SRC_UTIL_FREELIST_INDEX_ALLOCATOR_HPP
#include <expected>
#include <iterator>
#include <unordered_set>
#include <vector>
namespace util
{
/// A block allocator for allocation over a region
/// Given an inital number of blocks it will allocate blocks starting from 0
/// up to (but not including) that block
///! the null sentinel is ~0, not 0!
class BlockAllocator
{
public:
static constexpr std::size_t NullBlock = ~std::size_t {0};
public:
BlockAllocator(std::size_t numberOfBlocks);
~BlockAllocator() = default;
BlockAllocator(const BlockAllocator&) = delete;
BlockAllocator(BlockAllocator&&) = default;
BlockAllocator& operator= (const BlockAllocator&) = delete;
BlockAllocator& operator= (BlockAllocator&&) = default;
/// Allocates a new block from the allocator
///! returns BlockAllocator::NullBlock on failure
std::size_t allocate();
/// Frees a given block from the allocator, you must provide a valid,
/// allocated block
void free(std::size_t);
/// @returns whether or not the allocator has another block to
/// allocate
bool hasNext() const;
/// @returns the number of allocated blocks and the total number of
/// blocks
/// Number of allocated blocks is in the first member of the pair
/// Total number of blocks is in the second member
std::pair<std::size_t, std::size_t> peek() const;
/// Extends the size of the allocator, can only grow the size of the
/// allocator
void extend(std::size_t newSize);
private:
std::vector<std::size_t> free_blocks;
std::size_t total_blocks;
std::size_t next_free_block;
#ifndef NDEBUG
std::unordered_set<std::size_t> allocated_blocks;
#endif // NDEBUG
};
} // namespace util
#endif // SRC_UTIL_FREELIST_INDEX_ALLOCATOR_HPP
with a corresponding cpp file that looks like this
#include "block_allocator.hpp"
#include "util/assert.hpp"
#include <cstdlib>
namespace util
{
BlockAllocator::BlockAllocator(std::size_t numberOfBlocks)
: free_blocks {}
, total_blocks {numberOfBlocks}
, next_free_block {0}
#ifndef NDEBUG
, allocated_blocks {}
#endif // NDEBUG
{}
std::size_t BlockAllocator::allocate()
{
if (this->free_blocks.size() != 0)
{
/// Ok, the freelist has a an element that we can just pop and
/// return
const std::size_t newBlock = *this->free_blocks.crbegin();
this->free_blocks.pop_back();
#ifndef NDEBUG
this->allocated_blocks.insert(newBlock);
#endif // NDEBUG
return newBlock;
}
// We know that the freelist is empty, so we need to use our integers to
// figure out if a new block is still free
if (this->next_free_block >= this->total_blocks)
{
// There are no more blocks free
return BlockAllocator::NullBlock;
}
// Now we know that we have at least enough space for one more block to
// be allocated
const std::size_t newBlock = this->next_free_block;
this->next_free_block += 1;
#ifndef NDEBUG
if (this->allocated_blocks.contains(newBlock))
{
std::string fmtString = std::format(
"Block {} was already in the allocated blocks set!", newBlock);
std::puts(fmtString.c_str());
}
this->allocated_blocks.insert(newBlock);
#endif // NDEBUG
return newBlock;
}
void BlockAllocator::free(std::size_t block)
{
#ifndef NDEBUG
util::debugAssert(this->allocated_blocks.contains(block));
this->allocated_blocks.erase(block);
#endif // NDEBUG
this->free_blocks.push_back(block);
}
bool BlockAllocator::hasNext() const
{
return !this->free_blocks.empty()
|| this->next_free_block < this->total_blocks;
}
std::pair<std::size_t, std::size_t> BlockAllocator::peek() const
{
std::size_t usedBlocks = this->total_blocks
- (this->total_blocks - this->next_free_block
+ this->free_blocks.size());
return {usedBlocks, this->total_blocks};
}
void BlockAllocator::extend(std::size_t newSize)
{
if (this->total_blocks >= newSize)
{
abort();
}
this->total_blocks = newSize;
}
} // namespace util
thats a weird me thing, I often have large blocks of associated variables / classes so I want something to distinguish where they end and where the constructors begin
/// Simple implementation of the data structure as described in
/// https://github.com/stijnherfst/BrickMap
/// In general this is a two-level tree
/// Imagine a voxel volume of
class BrickMapVoxelVolume
{
public:
using VolumeVoxelPosition = util::Vec<std::size_t, 3>;
using BrickCoordinate = util::Vec<std::size_t, 3>;
using BrickLocalPosition = util::Vec<std::size_t, 3>;
struct BrickPointer
{
public:
/// Null also represents HomogeneousEmpty
static constexpr std::size_t Null = ~std::size_t {0};
static constexpr std::size_t HomogenousSolid = Null - 1;
public:
BrickPointer()
: ptr {BrickPointer::Null}
{}
BrickPointer(std::size_t ptr_)
: ptr {ptr_}
{}
bool isPtr() const
{
return this->ptr < HomogenousSolid;
}
std::size_t ptr;
};
public:
BrickMapVoxelVolume() = delete;
/// Constructs an empty BrickMapVoxelVolume with an edge length in
/// bricks
BrickMapVoxelVolume(std::size_t edgeLengthBricks);
~BrickMapVoxelVolume() = default;
BrickMapVoxelVolume(const BrickMapVoxelVolume&) = delete;
BrickMapVoxelVolume(BrickMapVoxelVolume&&);
BrickMapVoxelVolume& operator= (const BrickMapVoxelVolume&) = delete;
BrickMapVoxelVolume& operator= (BrickMapVoxelVolume&&);
/// Returns the previously stored voxel
bool updateVoxel(VolumeVoxelPosition, bool filled);
bool readVoxel(VolumeVoxelPosition) const;
std::uint64_t getStoredVoxelsEstimate() const;
void emitIntoObjStream(std::ostream&) const;
void emitIntoCsvPointStream(std::ostream&) const;
private:
BrickPointer allocateNewBrickPointer();
std::unique_ptr<BitMappedBrick[]> bricks;
util::BlockAllocator brick_allocator;
std::unique_ptr<BrickPointer[]> brick_map;
std::size_t edge_length_bricks;
};
another demo
as you can see its a big blocj
being bored during quarantine 💀
nope im (begrudgingly) self taught