392. Is Subsequence#5
Conversation
| CXX = g++ | ||
| CXXFLAGS = -Wall -Wextra -Werror -std=c++23 | ||
|
|
||
| all: $(NAME) |
There was a problem hiding this comment.
個人用なので気にしてないとかだったらすみません
ターゲット(コロンの左側)がファイル名ではない場合は以下のようにするといいです。
ターゲットが疑似ターゲットでファイルでないことを示します(runもファイルでないので同じようにします)
.PHONY: all
all: $(NAME)
...(また、一番上のターゲットはデフォルトターゲットでmakeだけで実行できるので(このような練習用リポジトリだと僕だったらrunを一番上にします(もしくは.DEFAULT_GOALを指定することでもできそう)))
There was a problem hiding this comment.
ありがとうございます。おっしゃる通り、.PHONY等は省略しています。
また、実行は明示的に行いたいので、自分は一番上にはしないことにしています。
|
|
||
| all: $(NAME) | ||
|
|
||
| $(NAME): $(SRC) step1.hpp step2.hpp step3.hpp |
There was a problem hiding this comment.
また、普通はヘッダーファイルはコンパイラに渡さずソースファイル(.cpp)から#includeのみします
There was a problem hiding this comment.
コンパイルコマンドは$(CXX) -o $(NAME) $(SRC) $(CXXFLAGS)ですので、ヘッダーファイルはコンパイラに渡していません。
ヘッダーファイルに変更があったときに再コンパイルされるようにしたいので、$(NAME)ターゲットに対するprerequisitesとして指定しています。
| public: | ||
| bool isSubsequence(const std::string &subsequence, const std::string &text) { | ||
| std::map<char, std::vector<size_t>> characterPositions; | ||
| for (size_t i = 0; i < text.size(); ++i) { |
There was a problem hiding this comment.
講師の方に聞きたいんですが、こういう時に選ぶiの型って左辺がsize_tである限り、(多くの場合は)何も考えずにsize_tを選ぶで大丈夫なんですかね?
自分がintをデフォルトで書いているので気になりました。
(intがだいたい32bit、size_tがだいたい64bitであるゆえに、intで値が表現できない場合を除いてです。
制約がtext.size()<=1e4となっているので)
(chromium code searchだと両方がありました)
There was a problem hiding this comment.
私はこれはどちらでも許容です。状況依存かもしれません。
int はその処理系で一番扱いやすい整数という意味ですね。size_t の方が新しいです。
| */ | ||
| public: | ||
| bool isSubsequence(const std::string &subsequence, const std::string &text) { | ||
| std::map<char, std::vector<size_t>> characterPositions; |
There was a problem hiding this comment.
vectorの代わりにsetでもいいんじゃないでしょうか(L49のlower_boundの書き方がすっきりします)
There was a problem hiding this comment.
なるほど、要素がuniqueであることが保証されているためsetでも良いのですね。
確かに、この方がスッキリと書けますね。
positions.lower_bound(searchPos);
https://en.cppreference.com/w/cpp/container/set
https://en.cppreference.com/w/cpp/container/set/lower_bound
setを使う場合はcharacterPositionsの初期化がO(N)ではなくO(NlogN)になりそうかなと思ったのですが、今回はそもそも挿入時にソート済みであることが確定しているためヒント付きの挿入操作を使うことでO(N)にすることができそうなので、この方が好ましいと思いました。ありがとうございます。
auto &positions = characterPositions[c];
positions.insert(positions.end(), i);
There was a problem hiding this comment.
setを使う場合はcharacterPositionsの初期化がO(N)ではなくO(NlogN)になりそうかなと思ったのですが、今回はそもそも挿入時にソート済みであることが確定しているためヒント付きの挿入操作を使うことでO(N)にすることができそうなので、この方が好ましいと思いました。
あー確かに…。すみません、そこ何も考えてませんでした(笑)
There was a problem hiding this comment.
現実的には log N の部分は、定数と同程度にしか気にしないと思います。N が 1T としても40とかですからね。
| for (char c : subsequence) { | ||
| auto &positions = characterPositions[c]; | ||
| auto found = | ||
| std::lower_bound(positions.begin(), positions.end(), searchPos); |
There was a problem hiding this comment.
二分探索が使えるときはだいたい尺取り法が使えることが多く、今回の場合そちらの方が読みやすいかなと思います
There was a problem hiding this comment.
おっしゃる通りですね。
step1は尺取り方で解いているのですが、こちらの解法はコメントにもあるとおりFollow Upに対するものになっています。
Follow upで聞かれているような状況(多数のクエリ(例えば 10^9 個以上)が与えられる場合)では、毎回 t 全体を線形走査するのは非効率になる可能性があります。
There was a problem hiding this comment.
あ、いえ、僕が言っているのはこちらです!
philip82148/leetcode-swejp#7 (comment)
@usatie さんの回答だと O(max(subsequence.size() * log (text.size()), text.size())) じゃないかなと思うのですが、こちらだとO(max(subsequence.size(), text.size()))になるかと。
There was a problem hiding this comment.
There was a problem hiding this comment.
あ、すみません、勘違いしてましたmm
characterPositionsの初期化を省ける前提なら、O(subsequence.size() * log (text.size())で処理できるということですか。
なるほどです
| if (characterPositions.empty()) { | ||
| for (size_t i = 0; i < text.size(); ++i) { | ||
| char c = text[i]; | ||
| characterPositions[c].push_back(i); | ||
| } | ||
| } |
There was a problem hiding this comment.
※講師役の方感覚が違ったら訂正していただきたいです
本当はこういうのはコンストラクタで初期化したいところなんですが、LeetCodeの性質上引数として渡されてしまいます。
つまり、「コンストラクタ(事前処理)で」「characterPositionsを初期化する」というのが「textが前と同じだったら」「characterPositionsを初期化する」となっているので、その思考のままで行くと、僕だったらif文内を別のメソッド(initCharacterPositions())に分けます。
if (条件式) initCharacterPositions(text);条件式のところはcharacterPositions.empty()で十分だと思いますが、もしロジックが変わっても(「textが前と同じだったら」をより丁寧に表現する必要がでてきた場合等)、ここを関数にして別にすればいいので分かりやすい/変更しやすいと思っています。
There was a problem hiding this comment.
副作用のある関数であるという点のほうが気になります。この関数はマルチスレッドで呼ばれた場合に競合状態になる可能性があります。競合状態を避けるため、副作用のある書き方は現実的な範囲でできるだけ避けたほうがよいと思います。また、どうしても副作用のある書き方をしなければならない場合は、適切にロックを取る必要があるかを考慮するのが良いと思います。
| if (initialized_data.count(text) == 0) { | ||
| initialize(text); | ||
| } |
There was a problem hiding this comment.
initialized_dataじゃ何か分からないのでtext_to_character_positionsとかがいいと思います
| // https://cplusplus.com/articles/oz18T05o/ | ||
| // https://davekilian.com/cpp-type-erasure.html | ||
| bool isSubsequence(const std::string &subsequence, const std::string &text) { | ||
| auto is_subseq = [&](auto &&self, size_t i, size_t j) -> bool { |
There was a problem hiding this comment.
ラムダ関数にラムダ関数を渡すのはエンジニアのコード(競プロ以外)ではあまり見ないパターンという認識なんですが、どうですかね??
僕も再帰ラムダ関数使った方がすっきりすると良く思うんですけど、実際にはちゃんとしたメソッドや関数として書くべきなのかなと思っています
※講師役の方感覚が違ったら訂正していただきたいです
それは気にしない前提で話しますと、ラムダ関数というのはoperator()()メソッド(演算子のオーバーロード)を持った無名クラスのオブジェクトで、フィールドとしてキャプチャした変数を持ちます。
なので、もし渡すなら&&(右辺値参照)じゃなくて&(左辺値参照)で十分かなと思っています。
なお、もし&&(右辺値参照)にする際は関数呼び出しの際にself(move(self), ...)とするのがいいです。
以下右辺値と左辺値の説明
右辺値と左辺値はとてもややこしい問題なので簡単には説明できないです…
頑張って簡単に説明すると、左辺値は普通の変数など、右辺値は0,1,2,3...(数値)、true/false(boolean値)、'a'/"abc..."(文字、文字列)などです。
そして、変数に代入する前の関数の戻り値も右辺値になります。
auto /* resultは左辺値 */ result = /* ここでは右辺値 */ func1(/* ここでは右辺値 */ func2());この違いがあるのは、右辺値は自分しか使わないので破壊したり変更したりしてもいいもの、左辺値は他の人が使う可能性があるので破壊したり変更したりできないもの、みたいなのがざっくりとした説明です。
そしてややこしいのはそれを直接扱わず(メモリを誰かに持ってもらって)参照するのが右辺値参照や左辺値参照ですが、参照自体は変数なので全て左辺値になることです。(笑)
auto /* selfは右辺値参照だけど左辺値 */ &&self = <右辺値>なので、右辺値参照という左辺値を右辺値として渡すために、move()による変換が必要になります。
self(move(self), ...)より正確な説明は色々記事を見てみるといいと思います
https://qiita.com/luftfararen/items/1de032bc6e3eb69ca672
There was a problem hiding this comment.
あ、僕が一つ間違えていましたが、auto &&は右辺値参照じゃなくてユニバーサル参照というものですね。
なので今回の場合はauto &&は左辺値参照扱いになっているという...
なのでこのままでも大丈夫ですね…(多分?)
There was a problem hiding this comment.
はい、大丈夫だと思います。
Qiitaの記事もいいですが、cppreferenceなどのドキュメントを読まれることをお勧めします。(私も読んでいる最中ですが!)
右辺値参照で渡したほうがラムダのコピーのオーバーヘッドがないので、こうした方が良いのか?
とも思い実行してみたところ一応動くようなのですが、これは引数が評価されるタイミングでis_subseqのmoveが行われ、その後にis_subseqを呼ぶことになっているので未定義動作なのでは?とも感じます。
だとすれば、そもそもauto const &selfという宣言でいいのではという気にもなってきますね。
return is_subseq(std::move(is_subseq), subsequence.size(), text.size());
There was a problem hiding this comment.
あ、ちなみに大丈夫というのはエンジニアが見慣れたコードかという意味で大丈夫かという話だったのですが…。
(個人的に左辺値参照で足りるのにユニバーサル参照にしているのが気持ち悪い。けど気にしすぎかもしれない)
※講師役の方にコメントを願いたいです。
右辺値参照も左辺値参照も、どちらも参照なのでコピーは通常されないですよ
(コンストラクタを呼び出して新しくオブジェクトを作るときは別で、左辺値で渡したらコピーされて、右辺値で渡したらムーブします)
There was a problem hiding this comment.
あ、すみません、完全に間違えました。
「ラムダのコピーのオーバーヘッドがないので、」という部分はauto selfとしていた時に考えていたことで、先ほどはそれとごっちゃになってしまっていました。
わかります。
左辺値参照で足りるのにユニバーサル参照にしているのが気持ち悪い。
There was a problem hiding this comment.
自分自身を引数に与えるのは、ラムダ計算の理論的な文脈では見ますが、プロダクションコードで見たことはない気がしますね。
もっとも、私が C++ 書いていた頃は、まだ C++ lambda があまり一般的ではない時代と場所だったので、そもそも複雑なラムダを書かないという発想でした。
C++23 から explicit object parameter が使えるようで、LeetCode も対応しているようです。
auto を含む lambda は generic lambda なのでユニバーサル参照になるが、使っていないので普通のリファレンスでいいのではということですね。(そもそも、私はあまり複雑なラムダ書きたくないですが、これは古い人だからですね。)
| if (found == positions.end()) { | ||
| return false; | ||
| } | ||
| searchPos = *found; |
There was a problem hiding this comment.
当初はssize_t searchPos = -1とstd::upper_boundで書いていたのですが、lower_boundに変更した際にsearchPos = *found + 1と変更しないといけないところを見落としていました。
以下、Solution1〜Solution3はすべて、同じ間違いをしています。
| // ラムダでの再帰関数がうまく書けなくて、std::functionを使うことになった | ||
| // 書ける場合と書けない場合の違いは何だろうか? | ||
| bool isSubsequence(const std::string &subsequence, const std::string &text) { | ||
| std::function<bool(size_t, size_t)> is_subseq = [&](size_t i, |
There was a problem hiding this comment.
ちょっと詳しい方がいたら触れてもらおうと思っていたのですが、もしかしたら触れられずに終わってしまうかもしれないので一応コメントしておきます。
std::functionを使う場合、auto func = ...の場合と違ってキャプチャされた変数を保存するメモリが動的に(ヒープに)確保される可能性があります。
それに、呼び出しも少しオーバーヘッドがあります。
なので使えるときはauto func = ...の形式がいいと思います(なおこれで再帰を書くには引数に渡すしかありません)。
https://stackoverflow.com/questions/46163607/avoid-memory-allocation-with-stdfunction-and-member-function
https://uchan.hateblo.jp/entry/2019/03/17/070736
https://timsong-cpp.github.io/cppwp/n4950/function.objects (規格書)
※良いリファレンスが見つけられずすみません。実装を読むのが早いかもしれません。
There was a problem hiding this comment.
ちなみに多分ですが、再帰が書けるか否かは左辺と右辺が厳密には違うものであることに起因しているんじゃないですかね。
L151の右辺は関数オブジェクトですが、左辺はそれを参照する関数ポインタですので。
(L151は右辺を第一引数にfunctionのコンストラクタを呼び出す、という文の糖衣構文になっています。)
auto func = ...だと左辺と右辺が同じものになります。
型推論に起因する問題な気がしてきました。
※C++は言語仕様巨大すぎて、どんなにC++学んでも初心者を抜け出せないというミームがあるくらいなので、最初の方はここら辺はあんまり理解していなくてもいいんじゃないかと個人的には思っています。
There was a problem hiding this comment.
| if (characterPositions.empty()) { | ||
| for (size_t i = 0; i < text.size(); ++i) { | ||
| char c = text[i]; | ||
| characterPositions[c].push_back(i); | ||
| } | ||
| } |
There was a problem hiding this comment.
副作用のある関数であるという点のほうが気になります。この関数はマルチスレッドで呼ばれた場合に競合状態になる可能性があります。競合状態を避けるため、副作用のある書き方は現実的な範囲でできるだけ避けたほうがよいと思います。また、どうしても副作用のある書き方をしなければならない場合は、適切にロックを取る必要があるかを考慮するのが良いと思います。
| size_t m = subsequence.size(); | ||
| size_t n = text.size(); | ||
| // is_subseq[i][j] : subsequence[0..i) is a subsequence of text[0..j) | ||
| std::vector<std::vector<bool>> is_subseq(m + 1, |
There was a problem hiding this comment.
std::vector については何度か議論があったと思います。それらを意識したうえで使うのであれば大丈夫だと思います。過去のコメントを探してみることをお勧めいたします。
| for (size_t j = 0; j <= n; ++j) { | ||
| is_subseq[0][j] = true; | ||
| } | ||
| for (size_t i = 1; i <= m; ++i) { |
問題 : https://leetcode.com/problems/is-subsequence
言語 : C++
lambdaでの再帰関数の書き方について、今までは適当に拾ってきたサンプルコードを書き換えていたのですが、ちゃんと調べようとしてみたら知らないことがたくさん出てきて、まだ全部は理解できていません。
ChatGPT-o3-mini-highに色々と教えてもらったのですが、同じくC++でやられている方にはもしかしたら以下のような会話の内容が参考になるかもしれません。
https://chatgpt.com/share/67a67173-839c-8000-913d-ca67b6aa6859