Skip to content

Conversation

@jbfuehrer
Copy link

@jbfuehrer jbfuehrer commented Mar 18, 2019

I tried to improve the (logical) const-correctness of the library as methods such as Vocabulary::transform don't modify any internal data from the library user's perspective(*). Since it's not const-qualified however, it enforces its non-constness on user methods which then snowballs further from there and has potential to makes code somewhat unclean.

I therefore templated the internal Block structure on a type that's either char or const char (could further be enforced via type traits) and then conditionally const-qualified all accesses to the internal block structure depending on whether or not the type itself was templated on a const type on construction (which is enforced in const methods, see below).

This results in this being valid:

void MyClass::foo()
{
  Block<char> block=getBlock(0);
  block.setLeaf(true);
  block.nonConstMethod();
  block.constMethod();
}

void MyClass::foo() const
{
  Block<const char> block=getBlock(0);
  bool is_leaf = block.isLeaf();
  block.constMethod();
}

while any of these don't compile

void MyClass::foo() const
{
  Block<char> block = getBlock(0);  // error: getBlock returns Block<const char>
  Block<const char> block=getBlock(0);  // ok
  block.setLeaf(true);  // error: can't modify through const object
  block.nonConstMethod();  // same

  auto* binfo = block.getBlockNodeInfo(0);
  binfo->nonConstMethod();  // error: constness is also forwarded to BlockNodeInfo
}

C++17 can even automatically deduce Block's template type from the ctor argument but since I'm not sure what the requirements for the library are, I didn't rely on this. Can you please tell me which platforms and/or language standard you are offering to support so I can make sure it compiles for these as well?

(*) The one exception to this rule is the cpu_info but since this is an implementation detail that imo should be transparent to the user and not affect (logical) constness of the API interface, I qualified it as mutable.

@rmsalinas
Copy link
Owner

Hi, I need the library to be available as many platforms and compilers as possible. So far I ahve tested (gcc with c++11, clang, msvc)

@jbfuehrer
Copy link
Author

Well, this is all standard cpp so it's rather a question of what the lowest standard version is you'd like to support. If you'd still like to support C++11 I can rewrite the C++14 parts.

… sometimes assigns the same visual word as center point leading to empty/meaningless visual words
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants