s2e-core icon indicating copy to clipboard operation
s2e-core copied to clipboard

Consider namespace separation rule

Open 200km opened this issue 2 years ago • 14 comments

Overview

Consider namespace separation rule

Details

Discussed at
https://github.com/ut-issl/s2e-core/pull/321#discussion_r1104400947

Conditions for close

NA

Supplement

NA

Note

NA

200km avatar Feb 14 '23 13:02 200km

ライブラリディレクトリが整理されたので新しい構成に合わせて名前空間も見直す。 ディレクトリ構成に合わせるなら、次のようになるか?

とりあえず、今存在しているlibraに焦点を当てて考える。libraryディレクトリがほぼそのままmath_physicsに変わったので、次のような感じか?

namespace math_physics{
  namespace math{
    Class Vector;
    Class Matrix;
     他も同様
  }
  namespace atmosphere{
    Class HarrisPriester;
    Class Nrlmsise00;
  }
}

もしくは、

namespace math_physics{
  Class Vector;
  Class Matrix;
  Class HarrisPriester;
  Class Nrlmsise00;
}

namespace math{
  Class Vector;
  Class Matrix;
   他も同様
}
namespace atmosphere{
  Class HarrisPriester;
  Class Nrlmsise00;
}

などもあり得るのか?コメントいただきたい。

200km avatar Jun 29 '24 01:06 200km

math_physics は名前が長い割に責務が明確ではない(mathphysics それぞれの責務のモノがなんだかんだ独立して存在する)ので、

namespace math{
  Class Vector;
  Class Matrix;
   他も同様
}
namespace atmosphere{
  Class HarrisPriester;
  Class Nrlmsise00;
}

のような分割はかなり自然に思います。

sksat avatar Jul 11 '24 08:07 sksat

コメントありがとうございます。私も下記のものは良いと思っているので、この方向で進めようと思います。

namespace math{
  Class Vector;
  Class Matrix;
   他も同様
}
namespace atmosphere{
  Class HarrisPriester;
  Class Nrlmsise00;
}

200km avatar Jul 11 '24 09:07 200km

#658 をreviewしてて感じたことです。

Orbit class があるのに,それは orbit namespace 外なことに少し違和感がありました。 library directory がなくなって math_physics が他の src 下の directory と同列になったこともあり,他全体についても namespace があるとよい気がしました。例えば component::Component disturbance::GravityGradient といった感じです。

この際に,math_physics/orbit と dynamics/orbit とを区別する必要が出てきます。 自然なのは directory に即した区別で,例えば math_physics::orbit::OrbitalElements dynamics::orbit::RelativeOrbit のような感じになります。 ただ,これは上記で棄却された案で,冗長でもあるので,悩ましいですね。

conjikidow avatar Aug 11 '24 23:08 conjikidow

@conjikidow コメントありがとうございます。とりあえず今出ているPR消化してから改めて整理して考えたいのでレビューお願いします。

200km avatar Aug 12 '24 13:08 200km

@conjikidow こちらの議論ですが、今は次のような状況です。

  • 元々namespaceは旧libraryにしかついていなかったので、今の整理も旧library関係のもの(主にmath_physics)になっている

旧library以外のもの全てにnamespaceをつけるかの議論がまず必要だと思います。 全てにnamespaceをつける利点・欠点などは整理したいですが教えてもらえると嬉しいです。

参考として、dynamics/orbitmath_physics/orbitの差としては、次のようなことを考えています。

  • 前者は真値系としての実体であり後者を利用して実体化させたものである。
  • 後者はあくまでいろんな人に使われる側であり、s2e-coreでは前者に使われている。ユーザーによってはこれを利用して推定系や制御系など真値系と別のestimate/orbitなどを作る可能性もある

200km avatar Aug 25 '24 08:08 200km

@200km ありがとうございます。

元々namespaceは旧libraryにしかついていなかったので、今の整理も旧library関係のもの(主にmath_physics)になっている

この経緯は理解しているため,今進められているもの自体についてはそのままで構いませんが,今後のさらなるアップデートに向けて,という意図でした。 その上で

旧library以外のもの全てにnamespaceをつける

のほうが良いと考えています。

全てにnamespaceをつける利点・欠点などは整理したいですが教えてもらえると嬉しいです。

ですが,

  • 全てにnamespaceをつける利点
    • 衝突を避けられる
      • 現在,旧library以外のものはglobalになっているため,名前衝突のリスクがある
        • 例えば dynamics/orbit math_physics/orbit estimate/orbit の中で同じ名前のものを作りたい可能性もあるが,現状はそれができず名前を変えることが必須である
      • ユーザーが独自の外部ライブラリを利用する場合にも衝突の可能性がある
    • ユーザーの自由度が増す
      • 上に関連するが,現状はglobalになっているために,ユーザーがcore側で提供されているクラスや関数と同名のものを定義することができない
        • 正確にはユーザーが独自の名前空間を用いれば定義可能だが,その場合もcore側はglobalなので ::ClassName のようにglobal名前空間を明示的に示す必要があり,ambiguousである
      • 特にその名前が一般的であるほど名前に自由度がない
        • 例えば独自の電池を扱いたい場合,すでにcoreで Battery が定義されているため,それを継承したクラスを作ろうとしても Battery は使えない
        • 命名規則に反する Bat BAT のようにするか,CustomBattery などのようにするか,あるいは製品名にするか,など,他にも可能な選択肢はあるが,ユーザーにそれを強要することになる
    • 構造が明確になり可読性が上がる
      • 例えば書いていただいたような違いが dynamics/orbitmath_physics/orbit があるが,現状それらの区別はクラス名からしかわからない
        • math_physicsは旧libraryに該当するので実際にはそうではないが,ユーザーが estimate/orbit を定義する場合にはそうなる
        • 例えば math_physics::orbit dynamics::orbit estimate::orbit のようにすれば,それぞれの所属が明確になる(もちろんestimate::orbitについてはユーザーの自由だが)
      • 特にこの構造をファイル構造と一致させた場合,関連するコードがどこにあるかが一目でわかるようになる
  • 全てにnamespaceをつける欠点
    • 修正コストがかかる
      • core はもちろん,現在のユーザーも作業が必要になる
    • 後方互換性がなくなる
      • ユーザーがその作業をしない限りcoreの後方互換性が効かない
    • 冗長になる
      • 単純に名前が長くなる

といったところでしょうか。

小規模なプロジェクトでは必ずしもそうではないですが,大規模で長期的なプロジェクトにおいては維持や拡張性の観点から名前空間を導入するほうが一般的だと思っています。 一方で欠点に挙げたコストがかなり大きく無視できないので,そことのトレードオフかなと思います。

conjikidow avatar Aug 25 '24 23:08 conjikidow

具体的にありがとうございます。基本的な利点・欠点の整理について、書かれている内容は同意します。あとは、程度問題で何に重きを置くかですね。

現状のaobcなどを考えて個別の事象にどう対処しているかを考えながら議論を深めたいですね。

例えば独自の電池を扱いたい場合,すでにcoreで Battery が定義されているため,それを継承したクラスを作ろうとしても Battery は使えない

などは、製品名にすることを推奨しているという気持ちです。(aobcなどは全てそう) また、これをs2e-core側でcomponent::batteryとしても、一般性が高いままなので、ユーザーも結局component::battery使いたいとなる気もしています。 ユーザー側が、my_component::batteryなどにするなら、my_batteryとするのとあまり変わらないですから難しいですね。s2e-core側をcore_component::batteryのように一般性を下げる方が良いんですかね。

dynamics::orbitも、ユーザーからしたら推定器の中のdynamicsだから、dynamics::orbitと名付けたいなというのなども十分あり得る気がしています。

この辺り、s2e-core側で良い名前空間をつけないと、欠点にあるような労力をかけたのに、結局利点にあるようなスマートな名前衝突回避にならず、名前空間使わずに衝突回避した時と変わらないとかになってしまいそうなのを危惧しています。

これを根本解決するには、全てを s2e-core::hoge::fugaのように絶対ユーザーがつけないようにするとかしか無いんですかね。

200km avatar Aug 26 '24 01:08 200km

ちなみに、欠点の後方互換性と名前が長くなる件は、あまり重要視していません。

  • どのみちこのMajor updateはいろんな後方互換性をなくしている
  • 名前が長くなる件はs2e-coreでは許容して、略語なども使わないという方針になっている

200km avatar Aug 26 '24 01:08 200km

ありがとうございます。

これをs2e-core側でcomponent::batteryとしても、一般性が高いままなので、ユーザーも結局component::battery使いたいとなる気もしています。 ユーザー側が、my_component::batteryなどにするなら、my_batteryとするのとあまり変わらないですから難しいですね。s2e-core側をcore_component::batteryのように一般性を下げる方が良いんですかね。

この辺り、s2e-core側で良い名前空間をつけないと、欠点にあるような労力をかけたのに、結局利点にあるようなスマートな名前衝突回避にならず、名前空間使わずに衝突回避した時と変わらないとかになってしまいそうなのを危惧しています。

これを根本解決するには、全てを s2e-core::hoge::fugaのように絶対ユーザーがつけないようにするとかしか無いんですかね。

これはおっしゃる通りで,究極は

全てを s2e-core::hoge::fugaのように

core全体を特定の名前空間下に置く必要があるという結論になると思います。 これ自体は不自然ではなく,std boost Eigen 等々,主要なライブラリでも一般的な方法です。

ただ,この議論と,個々のクラスの名前空間をどこまで深くするかは別の議論になる気がしますね(現状において全て s2e-core 下に置くだけという方針もあり得るため)。

いずれにせよ,少なくともv8は外部ライブラリ周りの修正がメインのため,今回修正いただいたところで閉じて良いと思います。

あとは、程度問題で何に重きを置くかですね。

の通りで絶対正しい答えがあるわけではないですし,すぐに結論が出るものでもないと思うので。 自分が道を外したコメントをしてしまって恐縮ですが,当初の旧ライブラリ関連については結論が出ているので,こちらは完了で問題ないかと思います。

conjikidow avatar Aug 30 '24 09:08 conjikidow

修正方針が決まれば修正自体は簡単なので、今回やってしまっても良いかなと思っています。

s2e::ディレクトリ名

のようなルールで行くとすぐ決めて良いならそれでやってしまいます。いかがでしょうか?

200km avatar Sep 02 '24 00:09 200km

ありがとうございます。

欠点の後方互換性と名前が長くなる件は、あまり重要視していません

ということであれば,自分も s2e::ディレクトリ名 で良いかなと思います。

conjikidow avatar Sep 02 '24 01:09 conjikidow

では、それでPR出してみようと思います。

200km avatar Sep 02 '24 05:09 200km

ありがとうございます,よろしくお願いします。

conjikidow avatar Sep 02 '24 06:09 conjikidow