This step is the preparation of allowing lvalue in map/motion clause.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 47056 Build 49821: arc lint + arc unit
Event Timeline
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
15246 | emitErrorMsg() here? | |
15247 | No need cast here. | |
15248 | Better to use IgnoreParenCasts() to handle explicit casting to the base class. | |
15255 | Just Visit(E) | |
15260 | Default initializer here | |
15262 | Default initializer | |
15263–15265 | Better to use = <init>; here. Just a nit. | |
15278 | Just Visit(E) | |
15283 | Just Visit(E) | |
15300 | Just Visit(E) | |
15319 | Visit(E) | |
15336–15337 | E->IgnoreParensCasts() | |
15338 | Need to check that AE->getIdx() is not value dependent, otherwise it may crash | |
15369–15370 | Just Visit(E) | |
15402–15411 | Also, need to be sure that the expressions here are not value-dependent. | |
15445–15474 | Just add: boo VisitStmt(Stmt *) { emitErrorMsg(); return false; } instead of all these functions, returning false as a result. | |
15475–15477 | No need for else here, just unconditional return nullptr. |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
15338 | It seems Clang would catch the error before we do the analysis: orig.cpp:6:24: error: array subscript is not an integer #pragma omp target map(a[b]) ^ ~ orig.cpp:15:3: note: in instantiation of function template specialization 'gg<int, double>' requested here gg<int, double>(a, c); ^ orig.cpp:8:5: error: array subscript is not an integer a[b] = 10; ^ ~ 2 errors generated. Also, if we still need it, do we also check type dependent? |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
15338 |
|
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
15319 | return RelevantExpr || Visit(E);? | |
15346 | return RelevantExpr || Visit(E);? | |
15357–15358 |
| |
15372–15373 | return RelevantExpr || Visit(E);? | |
15418–15427 | Need a check for value-dependent OASE->getLowerBound() | |
15420–15421 |
| |
15443 | Just Visit(E); | |
15452 | Do you really need this function? | |
15461–15477 | Use \\\ style of comment here |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
15452 | Removed the function. |
Did you try to run the tests? I would suggest adding a test, at least, where a function is mapped. We should see the error message here. Seems to me, we don't have this one.
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
15400–15403 | Same here, try to reduce complexity | |
15402–15403 | Just a nit. Try to reduce the structural complexity here by using logical exressions, if (!OASE->getLength()->isValueDependent() && OASE->getLength()->EvaluateAsInt(...) && !ResultR.Val.getInt().isOneValue()). Also, you can |
We already have test for err_omp_invalid_map_this_expr, note_omp_invalid_subscript_on_this_ptr_map, etc.. in target_message.cpp line 44 without checking for value dependence. Do you mean that I should add a test for the check of value dependence? In that case, we don't print any messages.
No. But previously, if we tried to map a function, not a variable, it would be skipped silently. With this patch, this behavior will change. I suggest adding a test with mapping a function to check that error message is emitted. Something like:
int foo(); #pragma omp target map(foo) // <- must be an error here
And I just asked if you tried to run the tests with this patch. Did the result change or it is the same?
Got it, I'll add test for function mapping. I've run the test and this patch passed all the test.
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
15357–15359 | Simplify this nested ifs too, please. | |
clang/test/OpenMP/target_messages.cpp | ||
53–57 | You mapped CallExprs here, not DeclRefs | |
119–125 | same here, mapped CallExprs |
No problem. But I don't see the changes in the test.
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
15473 | Remove braces here, the substatement is single line. |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
15452 | Was this function intended to be removed? As far as I can tell it was not and it seems to be the source of an issue I am having: expected addressable lvalue in 'map' clause |
emitErrorMsg() here?