Fixed FP for thread_local_initializer_can_be_made_const for os_local
#12186
Conversation
|
r? @xFrednet (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
the
S-waiting-on-review
|
r? @llogiq |
|
Failed to set assignee to
|
|
I am not sure how to verify this. @petrochenkov since you reported it, if you don't mind, could you verify? |
|
I'd agree that this should have some kind of test, or alternatively an explanation why it cannot be tested. |
|
@llogiq does the clippy test-suite include a cygwin backend? A potential issue is that we might experience different things for specific platforms which makes clippy development harder. |
|
No, we currently don't have cygwin. I'd like to understand the difference that makes to the lint, though. |
|
The difference relates to which implementation is used. Threadlocal has 3 implementations whivh get conditionally compiled based on the platform. For windows, threadlocal uses os native threads, for linux it uses llvm and For the former, we always have an init function regardless of whether the expression is const. If threadlocal is const, the function is a const fn. For thread local fast, if const is used, the value is used directly by threadlocal, and we don't have the init fn. if there is no const, an init fn is used. For static, there is no init fn either. So with the lint we check if we are in init fn. Does this make sense? |
|
Wouldn't Cygwin (once or maybe if it's added) use Windows implementation since it runs on Windows? Or does it provide some wrappers to emulate Unix thread-local? |
|
Cygwin will use the os native implementation once added, yes. |
|
We have a few windows-specific tests already (I believe we used to put them in a "ui.windows" test subfolder or some such. Could that work in this case? |
|
@llogiq I don't think the tests are actually running in the build scripts. |
m-rph
force-pushed
the
hide-thread-local
branch
from
2ad7917
to
d6cd0a4
Compare
|
That's ok, I have a Windows partition I can run those tests on locally. |
|
Ok, so in theory if you run the code as-is, right now, it should fail because of differences between |
…on_send_sync lint.
Checks for the suspicious use of OpenOptions::create() without an explicit OpenOptions::truncate(). create() alone will either create a new file or open an existing file. If the file already exists, it will be overwritten when written to, but the file will not be truncated by default. If less data is written to the file than it already contains, the remainder of the file will remain unchanged, and the end of the file will contain old data. In most cases, one should either use `create_new` to ensure the file is created from scratch, or ensure `truncate` is called so that the truncation behaviour is explicit. `truncate(true)` will ensure the file is entirely overwritten with new data, whereas `truncate(false)` will explicitely keep the default behavior. ```rust use std::fs::OpenOptions; OpenOptions::new().create(true).truncate(true); ```
Also rebase and fix conflicts
- New ineffective_open_options had to be fixed. - Now not raising an issue on missing `truncate` when `append(true)` makes the intent clear. - Try implementing more advanced tests for non-chained operations. Fail
…e contains dashes
Prefixing a variable with a `_` does not mean that it will not be used. If such a variable is used later, do not warn about the fact that its initialization does not have a side effect as this is fine.
Partial rewrite of `unused_io_account` to lint over Ok(_). Moved the check to `check_block` to simplify context checking for expressions and allow us to check only some expressions. For match (expr, arms) we emit a lint for io ops used on `expr` when an arm is `Ok(_)`. Also considers the cases when there are guards in the arms. It also captures `if let Ok(_) = ...` cases. For `Ok(_)` it emits a note indicating where the value is ignored. changelog: False Negatives [`unused_io_amount`]: Extended `unused_io_amount` to catch `Ok(_)`s in `If let` and match exprs.
`os_local` impl of `thread_local` — regardless of whether it is const and unlike other implementations — includes an `fn __init() -> EXPR`. Existing implementation of the lint checked for the presence of said function and whether the expr can be made const. Because for `os_local` we always have an `__init()`, it triggers for const implementations. The solution is to check whether the `__init()` function is already const. If it is `const`, there is nothing to do. Otherwise, we verify that we can make it const.
m-rph
force-pushed
the
hide-thread-local
branch
from
7df69b7
to
2829416
Compare
os_localimpl ofthread_local— regardless of whether it is const and unlike other implementations — includes anfn __init() -> EXPR.Existing implementation of the lint checked for the presence of said function and whether the expr can be made const. Because for
os_localwe always have an__init(), it triggers forconstqualified andconst-qualifiable.The solution is to check whether the
__init()function is already const. If it isconst, there is nothing to do. Otherwise, we can make itconst.changelog: [
THREAD_LOCAL_INITIALIZER_CAN_BE_MADE_CONST]: fixed FP foros_localimplementation ofthread_local_inner!.