ABataev added a comment.
Erroneous tests?
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:11320
+ bool MutexClauseEncountered = false;
+ llvm::SmallSet<OpenMPClauseKind, 2> EncounteredAtomicKinds;
for (const OMPClause *C : Clauses) {
----------------
tianshilei1992 wrote:
> ABataev wrote:
> > ABataev wrote:
> > > tianshilei1992 wrote:
> > > > ABataev wrote:
> > > > > tianshilei1992 wrote:
> > > > > > ABataev wrote:
> > > > > > > tianshilei1992 wrote:
> > > > > > > > ABataev wrote:
> > > > > > > > > SmallVector with 2 elements should be enough, no need to use
> > > > > > > > > SmallSet here.
> > > > > > > > `SmallVector` doesn't provide `contains`, which we use to check
> > > > > > > > if `OMPC_compare` and `OMPC_capture` exist at the same time.
> > > > > > > > Using `SmallVector` requires us to have a complicated
> > > > > > > > comparison, for example:
> > > > > > > > ```
> > > > > > > > (EncounteredAtomicKinds[0] == OMPC_compare &&
> > > > > > > > EncounteredAtomicKinds[1] == OMPC_capture) ||
> > > > > > > > (EncounteredAtomicKinds[1] == OMPC_compare &&
> > > > > > > > EncounteredAtomicKinds[0] == OMPC_capture)
> > > > > > > > ```
> > > > > > > > which is not as neat as `SmallSet`.
> > > > > > > Use llvm::find
> > > > > > That is linear time. Why it is better than `SmallSet`?
> > > > > Because there are only 2 elements, no?
> > > > It could have more. If user writes it like `atomic compare compare
> > > > capture capture capture`, then it will have 5 elements.
> > > > `SmallSet<T>::insert` returns a `pair` and we know if it is already in
> > > > the set, which I have not added it yet but I'll. `SmallVector` then is
> > > > not that easy to use. Of course you could say do `llvm::find`
> > > > beforehand but it's not neat.
> > > Is this correct at all?
> > You still can scan over the small vector and check if it has specified
> > clause already.
> It's not correct. We can easily check via the following code:
> ```
> if (!EncounteredAtomicKinds.insert(C->getClauseKind()).second) {
> // emit an error
> }
> ```
> Using `SmallVector` we need to:
> ```
> if (llvm::find(EncounteredAtomicKinds.begin(), EncounteredAtomicKinds.end(),
> C->getClauseKind() != EncounteredAtomicKinds.end()) {
> // emit an error
> } else {
> EncounteredAtomicKinds.push_back(C->getClauseKind());
> }
> ```
> It's obvious which one looks better and have better readability. I can't see
> any point of using `SmallVector` here, unless there is huge performance
> difference, which I really doubt that matters.
```
if (llvm::find(EncounteredAtomicKinds, C->getClauseKind() !=
EncounteredAtomicKinds.end()) {
// emit an error
} else {
EncounteredAtomicKinds.push_back(C->getClauseKind());
}
```
but ok, let's keep SmallSet
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116261/new/
https://reviews.llvm.org/D116261
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
