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

Str from bytes API cleanup, attempt 2 #7172

Closed
wants to merge 6 commits into from

Conversation

erickt
Copy link
Contributor

@erickt erickt commented Jun 16, 2013

This is an updated version of #7039, where I'm updating the bytes-to-str functions. This is the first pull request of a couple so it'll be easier to review.

The major features renames str::from_bytes* to str::from_utf8* to make it clear that the bytes must be utf8 encoded. Second, I've made str::from_bytes consume the input value, and added str::from_utf8_slice* to go from &[u8] into &str.

Finally, I've changed vec::bytes::memcmp to work with &[u8], and did some minor cleanup of code.

/// The byte slice needs to contain valid utf8.
pub unsafe fn from_utf8_slice<'a>(v: &'a [u8]) -> &'a str {
let (ptr, len): (*u8, uint) = ::cast::transmute(v);
cast::transmute((ptr, len + 1))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a bug. This makes the last hidden byte of &str point to potentially invalid memory.

In order to safely go from &[u8] to &str, the byte slice needs to contain an additional byte at the end (not neccessary 0)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching it. I'll remove from_utf8_slice and leave behind from_utf8_slice_with_null for when we won't access invalid memory.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that for slices it doesn't need to end in a null, any byte will do. A null there just means that as_c_str won't have to make a copy if you call that function, while requiring a null in general means you might have to always copy.

@Aatch
Copy link
Contributor

Aatch commented Jun 17, 2013

@erickt needs a rebase

@erickt
Copy link
Contributor Author

erickt commented Jun 17, 2013

@Aatch: It's been rebased.

@thestinger
Copy link
Contributor

Buildbot masterfully screwed this up by losing half the builders (compile exception slave lost). Doing a temporary close/reopen to unjam it.

@thestinger thestinger closed this Jun 21, 2013
@thestinger thestinger reopened this Jun 21, 2013
@thestinger
Copy link
Contributor

@erickt: sadly needs a rebase :(

@erickt
Copy link
Contributor Author

erickt commented Jun 25, 2013

I'm delaying this pull request to first get a fix in for #7235.

@erickt erickt closed this Jun 25, 2013
@erickt erickt mentioned this pull request Jul 23, 2013
flip1995 pushed a commit to flip1995/rust that referenced this pull request May 20, 2021
…tion, r=flip1995

Metadata collection monster eating deprecated lints

This adds the collection of deprecated lints to the metadata collection monster. The JSON output has the same structure with the *new* lint group "DEPRECATED". Here is one of fourteen examples it was able to dig up in Clippy's code:

```JSON
  {
    "id": "assign_op_pattern",
    "id_span": {
      "path": "src/assign_ops.rs",
      "line": 34
    },
    "group": "clippy::style",
    "docs": " **What it does:** Checks for `a = a op b` or `a = b commutative_op a` patterns.\n\n **Why is this bad?** These can be written as the shorter `a op= b`.\n\n **Known problems:** While forbidden by the spec, `OpAssign` traits may have\n implementations that differ from the regular `Op` impl.\n\n **Example:**\n ```rust\n let mut a = 5;\n let b = 0;\n // ...\n // Bad\n a = a + b;\n\n // Good\n a += b;\n ```\n",
    "applicability": {
      "is_multi_part_suggestion": false,
      "applicability": "MachineApplicable"
    }
  }
```

And you! Yes you! Sir or Madam can get all of this **for free** in Clippy if this PR gets merged. (Sorry for the silliness ^^)

---

See: rust-lang#7172 for the full metadata collection to-do list or to suggest a new feature in connection to it 🙃

---

changelog: none

r? `@flip1995`
flip1995 pushed a commit to flip1995/rust that referenced this pull request May 20, 2021
…o-alias, r=flip1995

Add `cargo collect-metadata` as an cargo alias for the metadata collection lint

This PR adds a new alias to run the metadata collection monster on `clippy_lints`. I'm currently using it to create the `metadata_collection.json` file and I plan to use it in the `deply.sh` script. Having it as a new alias enables us to simply use:

```sh
cargo collect-metadata
```

It sometimes requires running `cargo clean` before collecting the metadata due to caching. I'm still debating if I should include a cargo clean as part of the `run_metadata_collection_lint` test or not. Input on this would be greatly appreciated 🙃

That's it, just a small change that can be reviewed and merged in parallel to rust-lang#7214.

---

See: rust-lang#7172 for the full metadata collection to-do list or to suggest a new feature in connection to it.

---

changelog: none

r? `@flip1995` btw. feel free to pass these PRs one to other team members as well if you want.
flip1995 pushed a commit to flip1995/rust that referenced this pull request May 20, 2021
…, r=flip1995,camsteffen

Metadata collection monster searching for Clippy's configuration options

This PR teaches our lovely metadata collection monster which configurations are available inside Clippy. It then adds a new *Configuration* section to the lint documentation.

---

The implementation uses the `define_Conf!` macro to create a vector of metadata during compilation. This enables easy collection and parsing without the need of searching for the struct during a lint-pass (and it's quite elegant IMO). The information is then parsed into an intermediate struct called `ClippyConfiguration` which will be saved inside the `MetadataCollector` struct itself. It is currently only used to generate the *Configuration* section in the lint documentation, but I'm thinking about adding an overview of available configurations to the website. Saving them in this intermediate state without formatting them right away enables this in the future.

The new parsing will also allow us to have a documentation that spans over multiple lines in the future. For example, this will be valid when the old script has been removed:
```rust
/// Lint: BLACKLISTED_NAME.
/// The list of blacklisted names to lint about. NB: `bar` is not here since it has legitimate uses
(blacklisted_names: Vec<String> = ["foo", "baz", "quux"].iter().map(ToString::to_string).collect())
```

The deprecation reason is also currently being collected but not used any further and that's basically it.

---

See: rust-lang#7172 for the full metadata collection to-do list or to suggest a new feature in connection to it 🙃

---

changelog: none

r? `@flip1995`
cc `@camsteffen` It would be great if you could also review this PR as you have recently worked on Clippy's `define_Conf!` macro.
flip1995 pushed a commit to flip1995/rust that referenced this pull request May 20, 2021
…, r=flip1995

Adding the default lint level to the metadata collection

I noticed while working on the website adaption that the lint groups still had the `clippy::` prefix in the JSON output. This PR removes this prefix and adds a `level` field to each lint and with that simplifies the website display and saves performance.

The deprecated lints get are assigned to the level `none`. This is a bit different in comparison to the current lint list, but I believe that this will look better overall. Unless there is any argument against this :).

That's it just a small baby PR in comparison to the original monster ^^

---

See: rust-lang#7172 for the full metadata collection to-do list or to suggest a new feature in connection to it.

---

changelog: none

r? `@flip1995`
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jun 3, 2021
Early return from LintPass registration when collecting metadata

This speeds up the metadata collection by 2-2.5x on my machine. During
metadata collection other lint passes don't have to be registered, only
the lints themselves.

cc rust-lang#7172

r? `@xFrednet`

changelog: none
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jun 3, 2021
…-collection, r=flip1995

Adding the ability to invalidate caches to force metadata collection

This adds the discussed hack to touch `clippy_lints/src/lib.rs` to invalidate cargos cache and force metadata collection. I've decided to use the [`filetime`](https://github.com/alexcrichton/filetime) crate instead of the touch command to make is cross-platform and just cleaner in general. Looking at the maintainers I would say that it's a save crate to use xD.

---

cc: rust-lang#7172 I know this ID without looking it up now... This is not good

changelog: none

r? `@flip1995`
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jul 1, 2021
…p1995

Fixed broken deploy script due to multiline configuration docs

The deploy script on master currently runs into an error (See [log](https://github.com/rust-lang/rust-clippy/runs/2865828873)) due to the new configuration documentation added in rust-lang#7299. The current documentation collection for the configuration macro sadly doesn't support multiline doc comments. This will be changes in the future with the new metadata collector tracked in rust-lang#7172 For now we have to use `<br>` inside doc comments to add paragraphs.

This PR restricts `define_Conf!` macro to single lines and adds a comment explaining the reasoning behind it. It also adjusted the actual document parsing to fix a bug. (The parsing was automatically stopping on the first curly bracket, even if it was part of a doc comment).

changelog: none
flip1995 added a commit to flip1995/rust that referenced this pull request Jul 29, 2021
…ormat, r=flip1995

Adapting the lint list to Clippy's new metadata format

This is close to the end of a long living project to rewrite the lint metadata collection for Clippy. Progress on this has been tracked in rust-lang#7172. This PR adds one of the last missing puzzle pieces, the actual display of all the changes that have been done in the background. A preview can be seen here: [Clippy's lint list](https://xfrednet.github.io/rust-clippy/master/index.html)

The styling has been discussed on [zulip](https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/Styling.20feedback.20for.20Clippy's.20lint.20list/near/239601067) but is still open to suggestion.

Side note: It has been fun working on the website where we don't have unit tests and everything is just tested by playing around. However, it's good that this chaos is contained into this one part of Clippy. 🐛

---

Closes: rust-lang#1303
Closes: rust-lang#4310

This actually closes fewer things than I thought it would...

changelog: Reworked Clippy's [website](https://rust-lang.github.io/rust-clippy/master/index.html):
changelog: * Added applicability information about lints
changelog: * Added a link to jump to the specific lint implementation
changelog: * Adapted some styling and improved loading time

r? `@flip1995`
flip1995 added a commit to flip1995/rust that referenced this pull request Jul 29, 2021
…ion, r=flip1995

Update lint documentation to use markdown headlines

This PR updates all lint documentation to use markdown headlines. It additionally removed the *Known problems* section for lints without any problems. I've double-checked all automatic replacements, but a second pair of eyes is definitely appreciated!

I wasn't sure when you wanted to switch to the new metadata collection tomorrow, I therefore prepared this PR today. And that's it this is a standalone PR to keep the other related PRs reviewable.

changelog:  none

r? `@flip1995`

cc: rust-lang#7172

Note: This should be merged with the other metadata collection related PRs.
flip1995 pushed a commit to flip1995/rust that referenced this pull request Nov 23, 2021
Add Clippy version to Clippy's lint list

Hey, hey, the semester is finally over, and I wanted to get back into hacking on Clippy. It has also been some time since our metadata collection monster has been feed. So, this PR adds a new attribute `clippy::version` to document which version a lint was stabilized. I considered using `git blame` but that would be very hacky and probably not accurate.

I'm also thinking that this attribute can be used to have a `clippy::nightly` lint group which is allow-by-default that delays setting the actual lint group until the defined version is reached. Just something to consider regarding rust-lang#6623 🙃

This PR only adds the version to 4 lints to keep it reviewable. I'll do a followup PR to add the version to other lints if the implementation is accepted 🙃

![image](https://user-images.githubusercontent.com/17087237/137118859-0aafdfdf-7595-4289-8ba4-33d58eb6991d.png)

Also, mobile approved xD

![image](https://user-images.githubusercontent.com/17087237/137118944-833cf7fb-a4a1-45d6-9af8-32c951822360.png)

---

r? `@flip1995`

cc: rust-lang#7172

closes: rust-lang#6492

changelog: [Clippy's lint list](https://rust-lang.github.io/rust-clippy/master/index.html) now displays the version a lint was added. 🎉

---

Example lint declaration after this update:

```rs
declare_clippy_lint! {
    /// [...]
    ///
    /// ### Example
    /// ```rust
    /// // Bad
    /// let x = 3.14;
    /// // Good
    /// let x = std::f32::consts::PI;
    /// ```
    #[clippy::version = "pre 1.29.0"]
    pub APPROX_CONSTANT,
    correctness,
    "the approximate of a known float constant (in `std::fXX::consts`)"
}
```
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jun 4, 2022
Collect renamed lints

changelog: Display past names of renamed lints on Clippy's lint list

cc rust-lang#7172

r? `@xFrednet`
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jun 16, 2022
…ednet

Add lint output to lint list

changelog: Add the ability to show the lint output in the lint list

This just adds the logic to produce the output, it hasn't been added to any lints yet. It did help find some mistakes in some docs though 😄.

### Screenshots

<details>
<summary>A single code block</summary>

![single-code-block](https://user-images.githubusercontent.com/69764315/172013766-145b22b1-1d91-4fb8-9cd0-b967a52d6330.png)
</details>

<details>
<summary>A single code block with a "Use instead" section</summary>

![with-usage](https://user-images.githubusercontent.com/69764315/172013792-d2dd6c9c-defa-41e0-8c27-8e8e311adb63.png)
</details>

<details>
<summary>Multiple code blocks</summary>

![multi-code-block](https://user-images.githubusercontent.com/69764315/172013808-5328f59b-e7c5-4914-a396-253822a6d350.png)
</details>

This is the last task in rust-lang#7172 🎉.
r? `@xFrednet` (?)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants