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

oj-bundle with multiple include path #298

Closed
jellc opened this issue Sep 11, 2020 · 2 comments
Closed

oj-bundle with multiple include path #298

jellc opened this issue Sep 11, 2020 · 2 comments

Comments

@jellc
Copy link
Contributor

jellc commented Sep 11, 2020

最後に '-I' で指定したパスのみがコンパイルオプションで include path として渡されています(例えば引数を ['-I', '/path/to/Library1', '-I', '/path/to/Library2'] と渡すと、Library1 にパスが通らず Library2 には通る)。

parser.add_argument('-I', metavar='dir', type=pathlib.Path, dest='iquote', default=pathlib.Path.cwd(), help='add the directory dir to the list of directories to be searched for header files during preprocessing (default: ".")')

クラス 'Language' とその派生クラスのメソッド 'bundle' のシグニチャを変更する('basedir' の型を 'List[pathlib.Path]' にする)ことで対応しようと考えています。それによって生じると想定される不具合や、より良い対応などあれば教えて頂きたいです。

@kmyk
Copy link
Member

kmyk commented Sep 11, 2020

自分のライブラリと atcoder/ac-library とを同時に使いたい、みたいな動機でしょうか。これができるようになってよろこぶ人は多そうで、対応やってくれるならありがたいです。


クラス 'Language' とその派生クラスのメソッド 'bundle' のシグニチャを変更する('basedir' の型を 'List[pathlib.Path]' にする)ことで対応しようと考えています。それによって生じると想定される不具合や、より良い対応などあれば教えて頂きたいです。

「クラス 'Language' とその派生クラスのメソッド 'bundle' のシグニチャを変更する」のはよさそうです。しかし 'basedir' の型を 'List[pathlib.Path]' にするのはすこし不具合がありそうで、def bundle(self, path: pathlib.Path, *, basedir: pathlib.Path, options: Dict[str, Any]) -> bytes という形にするのが無難かなと思います。(options: Dict[str, Any] がきれいかと言われるとそうでもないので、実装していて良いものを思い付いたらそれを使ってもらってかまいません)

想定される不具合:

  1. basedir という語は「.verify-helper.git が置いてあるディレクトリ」の意味で使われています。これが複数になってしまうとたぶんいろいろ壊れます。
  2. include path の概念は C++ に特有のものです。逆に C++ にはない要求が他の言語から出てくることも予想できます。抽象クラス Language の側を C++ 専用の形に修正してしまうと、他の言語を実装するときに混乱します。

その他の選択肢:

  1. def bundle(self, path: pathlib.Path, *, basedir: pathlib.Path, cplusplus_include_paths: List[pathlib.Path]) -> bytes なら、変数名が cplusplus_* であればぎりぎり許されるし、いま bundle 機能に対応できてるのは C++ だけなのでたぶんセーフです
  2. CPlusPlusLanguage のコンストラクタに引数を増やす選択肢も存在しますが、あまりきれいではなさそう。コンパイル時のオプションの管理や切り替えは LanguageEnvironment の側の役割なので
  3. cplusplus_bundle.py の中で環境変数 CPLUSPLUS_INCLUDE_PATH を読みに行く実装も可能ですが、これは避けたいです。oj-bundle から呼ぶ分には困らないのですが、oj-verify から呼ぶときに複数の環境の切り替えのまわりで混乱するためです

@jellc
Copy link
Contributor Author

jellc commented Sep 11, 2020

ありがとうございます。設計の理解が不十分な提案で申し訳なかったです。

「クラス 'Language' とその派生クラスのメソッド 'bundle' のシグニチャを変更する」のはよさそうです。しかし 'basedir' の型を 'List[pathlib.Path]' にするのはすこし不具合がありそうで、def bundle(self, path: pathlib.Path, *, basedir: pathlib.Path, options: Dict[str, Any]) -> bytes という形にするのが無難かなと思います。(options: Dict[str, Any] がきれいかと言われるとそうでもないので、実装していて良いものを思い付いたらそれを使ってもらってかまいません)

ある程度汎用性、柔軟性がありそうなこの形をとろうと思います。

想定される不具合:

  1. basedir という語は「.verify-helper.git が置いてあるディレクトリ」の意味で使われています。これが複数になってしまうとたぶんいろいろ壊れます。

全ての特殊化を確認せずに言ってしまっていたのですが、 'other.py' を見てこれがマズそうなことに気づきました。意味論的にもおかしいですし、そもそも Not Implemented な言語を無視するのも将来性を考えるとおかしいですね、、

  1. include path の概念は C++ に特有のものです。逆に C++ にはない要求が他の言語から出てくることも予想できます。抽象クラス Language の側を C++ 専用の形に修正してしまうと、他の言語を実装するときに混乱します。

C++ 特有の概念だという認識を持っていませんでした。なるほど納得です。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants