Addressed points raised in review.

This commit is contained in:
Alexander Regueiro 2019-03-10 17:19:47 +00:00
parent d43966a176
commit d2b85323ad
13 changed files with 44 additions and 48 deletions

View File

@ -26,8 +26,7 @@ declare_clippy_lint! {
/// ```
pub ASSERTIONS_ON_CONSTANTS,
style,
"`assert!(true)` / `assert!(false)` will be optimized out by the compiler, \
and should probably be replaced by a `panic!()` or `unreachable!()`"
"`assert!(true)` / `assert!(false)` will be optimized out by the compiler, and should probably be replaced by a `panic!()` or `unreachable!()`"
}
pub struct AssertionsOnConstants;

View File

@ -258,9 +258,8 @@ fn check_text(cx: &EarlyContext<'_>, valid_idents: &FxHashSet<String>, text: &st
fn check_word(cx: &EarlyContext<'_>, word: &str, span: Span) {
/// Checks if a string is camel-case, i.e., contains at least two uppercase
/// letters (`Clippy` is
/// ok) and one lower-case letter (`NASA` is ok). Plural are also excluded
/// (`IDs` is ok).
/// letters (`Clippy` is ok) and one lower-case letter (`NASA` is ok).
/// Plurals are also excluded (`IDs` is ok).
fn is_camel_case(s: &str) -> bool {
if s.starts_with(|c: char| c.is_digit(10)) {
return false;

View File

@ -20,7 +20,7 @@ declare_clippy_lint! {
/// them leads to more readable code.
///
/// **Known problems:** Potential false negatives: we bail out if the function
/// has a where-clause where lifetimes are mentioned.
/// has a `where` clause where lifetimes are mentioned.
///
/// **Example:**
/// ```rust
@ -385,7 +385,7 @@ impl<'a, 'tcx> Visitor<'tcx> for RefVisitor<'a, 'tcx> {
}
}
/// Are any lifetimes mentioned in the where-clause? If yes, we don't try to
/// Are any lifetimes mentioned in the `where` clause? If so, we don't try to
/// reason about elision.
fn has_where_lifetimes<'a, 'tcx: 'a>(cx: &LateContext<'a, 'tcx>, where_clause: &'tcx WhereClause) -> bool {
for predicate in &where_clause.predicates {

View File

@ -1413,7 +1413,7 @@ fn check_for_loop_arg(cx: &LateContext<'_, '_>, pat: &Pat, arg: &Expr, expr: &Ex
}
}
/// Checks for `for` loops over `Option`s and `Results`
/// Checks for `for` loops over `Option`s and `Result`s.
fn check_arg_type(cx: &LateContext<'_, '_>, pat: &Pat, arg: &Expr) {
let ty = cx.tables.expr_ty(arg);
if match_type(cx, ty, &paths::OPTION) {
@ -1673,7 +1673,7 @@ fn check_for_mutation(
delegate.mutation_span()
}
/// Returns `true` if the pattern is a `PatWild` or an ident prefixed with `'_'`.
/// Returns `true` if the pattern is a `PatWild` or an ident prefixed with `_`.
fn pat_is_wild<'tcx>(pat: &'tcx PatKind, body: &'tcx Expr) -> bool {
match *pat {
PatKind::Wild => true,
@ -2336,7 +2336,7 @@ fn path_name(e: &Expr) -> Option<Name> {
fn check_infinite_loop<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, cond: &'tcx Expr, expr: &'tcx Expr) {
if constant(cx, cx.tables, cond).is_some() {
// A pure constant condition (e.g., while false) is not linted.
// A pure constant condition (e.g., `while false`) is not linted.
return;
}

View File

@ -322,7 +322,7 @@ fn check_single_match_opt_like(
ty: Ty<'_>,
els: Option<&Expr>,
) {
// list of candidate Enums we know will never get any more members
// list of candidate `Enum`s we know will never get any more members
let candidates = &[
(&paths::COW, "Borrowed"),
(&paths::COW, "Cow::Borrowed"),
@ -335,7 +335,7 @@ fn check_single_match_opt_like(
let path = match arms[1].pats[0].node {
PatKind::TupleStruct(ref path, ref inner, _) => {
// contains any non wildcard patterns? e.g., Err(err)
// Contains any non wildcard patterns (e.g., `Err(err)`)?
if !inner.iter().all(is_wild) {
return;
}
@ -354,7 +354,7 @@ fn check_single_match_opt_like(
}
fn check_match_bool(cx: &LateContext<'_, '_>, ex: &Expr, arms: &[Arm], expr: &Expr) {
// type of expression == bool
// Type of expression is `bool`.
if cx.tables.expr_ty(ex).sty == ty::Bool {
span_lint_and_then(
cx,

View File

@ -592,8 +592,7 @@ fn is_used(cx: &LateContext<'_, '_>, expr: &Expr) -> bool {
}
/// Tests whether an expression is in a macro expansion (e.g., something
/// generated by
/// `#[derive(...)`] or the like).
/// generated by `#[derive(...)]` or the like).
fn in_attributes_expansion(expr: &Expr) -> bool {
expr.span
.ctxt()

View File

@ -52,7 +52,7 @@ impl Pass {
///
/// ```ignore
/// if option.is_none() {
/// return `None`;
/// return None;
/// }
/// ```
///

View File

@ -197,7 +197,7 @@ struct VectorInitializationVisitor<'a, 'tcx: 'a> {
/// Contains the information.
vec_alloc: VecAllocation<'tcx>,
/// Contains, the slow initialization expression, if one was found.
/// Contains the slow initialization expression, if one was found.
slow_expression: Option<InitializationType<'tcx>>,
/// `true` if the initialization of the vector has been found on the visited block.

View File

@ -1,5 +1,4 @@
/// Returns the index of the character after the first camel-case component of
/// `s`.
/// Returns the index of the character after the first camel-case component of `s`.
pub fn until(s: &str) -> usize {
let mut iter = s.char_indices();
if let Some((_, first)) = iter.next() {

View File

@ -147,7 +147,7 @@ pub fn get_def_path(tcx: TyCtxt<'_, '_, '_>, def_id: DefId) -> Vec<&'static str>
.collect()
}
/// Checks if type is struct, enum or union type with given def path.
/// Checks if type is struct, enum or union type with the given def path.
pub fn match_type(cx: &LateContext<'_, '_>, ty: Ty<'_>, path: &[&str]) -> bool {
match ty.sty {
ty::Adt(adt, _) => match_def_path(cx.tcx, adt.did, path),
@ -155,7 +155,7 @@ pub fn match_type(cx: &LateContext<'_, '_>, ty: Ty<'_>, path: &[&str]) -> bool {
}
}
/// Checks if the method call given in `expr` belongs to given trait.
/// Checks if the method call given in `expr` belongs to the given trait.
pub fn match_trait_method(cx: &LateContext<'_, '_>, expr: &Expr, path: &[&str]) -> bool {
let method_call = cx.tables.type_dependent_defs()[expr.hir_id];
let trt_id = cx.tcx.trait_of_item(method_call.def_id());
@ -434,7 +434,7 @@ pub fn get_item_name(cx: &LateContext<'_, '_>, expr: &Expr) -> Option<Name> {
}
}
/// Gets the name of a `Pat`, if any
/// Gets the name of a `Pat`, if any.
pub fn get_pat_name(pat: &Pat) -> Option<Name> {
match pat.node {
PatKind::Binding(.., ref spname, _) => Some(spname.name),
@ -614,7 +614,7 @@ fn trim_multiline_inner(s: Cow<'_, str>, ignore_first: bool, ch: char) -> Cow<'_
}
}
/// Gets a parent expressions if any this is useful to constrain a lint.
/// Gets the parent expression, if any - this is useful to constrain a lint.
pub fn get_parent_expr<'c>(cx: &'c LateContext<'_, '_>, e: &Expr) -> Option<&'c Expr> {
let map = &cx.tcx.hir();
let hir_id = e.hir_id;
@ -727,7 +727,7 @@ pub fn is_expn_of(mut span: Span, name: &str) -> Option<Span> {
}
}
/// Returns the pre-expansion span if is this directly comes from an expansion
/// Returns the pre-expansion span if the span directly comes from an expansion
/// of the macro `name`.
/// The difference with `is_expn_of` is that in
/// ```rust,ignore
@ -749,7 +749,7 @@ pub fn is_direct_expn_of(span: Span, name: &str) -> Option<Span> {
}
}
/// Convenience function to get the return type of a function
/// Convenience function to get the return type of a function.
pub fn return_ty<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, fn_item: hir::HirId) -> Ty<'tcx> {
let fn_def_id = cx.tcx.hir().local_def_id_from_hir_id(fn_item);
let ret_ty = cx.tcx.fn_sig(fn_def_id).output();
@ -759,9 +759,9 @@ pub fn return_ty<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, fn_item: hir::HirId) -> T
/// Checks if two types are the same.
///
/// This discards any lifetime annotations, too.
// FIXME: this works correctly for lifetimes bounds (`for <'a> Foo<'a>` == `for
// <'b> Foo<'b>` but
// not for type parameters.
//
// FIXME: this works correctly for lifetimes bounds (`for <'a> Foo<'a>` ==
// `for <'b> Foo<'b>`, but not for type parameters).
pub fn same_tys<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, a: Ty<'tcx>, b: Ty<'tcx>) -> bool {
let a = cx.tcx.erase_late_bound_regions(&Binder::bind(a));
let b = cx.tcx.erase_late_bound_regions(&Binder::bind(b));
@ -871,8 +871,8 @@ pub fn iter_input_pats<'tcx>(decl: &FnDecl, body: &'tcx Body) -> impl Iterator<I
(0..decl.inputs.len()).map(move |i| &body.arguments[i])
}
/// Checks if a given expression is a match expression
/// expanded from `?` operator or `try` macro.
/// Checks if a given expression is a match expression expanded from the `?`
/// operator or the `try` macro.
pub fn is_try<'a>(cx: &'_ LateContext<'_, '_>, expr: &'a Expr) -> Option<&'a Expr> {
fn is_ok(cx: &'_ LateContext<'_, '_>, arm: &Arm) -> bool {
if_chain! {

View File

@ -241,12 +241,12 @@ impl<'a> Sugg<'a> {
}
/// Adds parenthesis to any expression that might need them. Suitable to the
/// `self` argument of
/// a method call (e.g., to build `bar.foo()` or `(1 + 2).foo()`).
/// `self` argument of a method call
/// (e.g., to build `bar.foo()` or `(1 + 2).foo()`).
pub fn maybe_par(self) -> Self {
match self {
Sugg::NonParen(..) => self,
// (x) and (x).y() both don't need additional parens
// `(x)` and `(x).y()` both don't need additional parens.
Sugg::MaybeParen(sugg) => {
if sugg.starts_with('(') && sugg.ends_with(')') {
Sugg::MaybeParen(sugg)
@ -282,7 +282,7 @@ impl<'a> std::ops::Not for Sugg<'a> {
/// Helper type to display either `foo` or `(foo)`.
struct ParenHelper<T> {
/// Whether parenthesis are needed.
/// `true` if parentheses are needed.
paren: bool,
/// The main thing to display.
wrapped: T,
@ -320,12 +320,13 @@ pub fn make_unop(op: &str, expr: Sugg<'_>) -> Sugg<'static> {
/// often confusing so
/// parenthesis will always be added for a mix of these.
pub fn make_assoc(op: AssocOp, lhs: &Sugg<'_>, rhs: &Sugg<'_>) -> Sugg<'static> {
/// Whether the operator is a shift operator `<<` or `>>`.
/// Returns `true` if the operator is a shift operator `<<` or `>>`.
fn is_shift(op: &AssocOp) -> bool {
matches!(*op, AssocOp::ShiftLeft | AssocOp::ShiftRight)
}
/// Whether the operator is a arithmetic operator (`+`, `-`, `*`, `/`, `%`).
/// Returns `true` if the operator is a arithmetic operator
/// (i.e., `+`, `-`, `*`, `/`, `%`).
fn is_arith(op: &AssocOp) -> bool {
matches!(
*op,
@ -333,9 +334,8 @@ pub fn make_assoc(op: AssocOp, lhs: &Sugg<'_>, rhs: &Sugg<'_>) -> Sugg<'static>
)
}
/// Whether the operator `op` needs parenthesis with the operator `other`
/// in the direction
/// `dir`.
/// Returns `true` if the operator `op` needs parenthesis with the operator
/// `other` in the direction `dir`.
fn needs_paren(op: &AssocOp, other: &AssocOp, dir: Associativity) -> bool {
other.precedence() < op.precedence()
|| (other.precedence() == op.precedence()
@ -414,9 +414,8 @@ enum Associativity {
}
/// Returns the associativity/fixity of an operator. The difference with
/// `AssocOp::fixity` is that
/// an operator can be both left and right associative (such as `+`:
/// `a + b + c == (a + b) + c == a + (b + c)`.
/// `AssocOp::fixity` is that an operator can be both left and right associative
/// (such as `+`: `a + b + c == (a + b) + c == a + (b + c)`.
///
/// Chained `as` and explicit `:` type coercion never need inner parenthesis so
/// they are considered

View File

@ -48,9 +48,10 @@ fn predicate<F: FnOnce(T) -> bool, T>(pfn: F, val: T) -> bool {
fn pred_test() {
let v = 3;
let sky = "blue";
// this is a sneaky case, where the block isn't directly in the condition, but is actually
, andadd some extra
// expressions to make sure linter isn't confused by them.
// This is a sneaky case, where the block isn't directly in the condition,
// but is actually nside a closure that the condition is using.
// The same principle applies -- add some extra expressions to make sure
// linter isn't confused by them.
if v == 3
&& sky == "blue"
&& predicate(

View File

@ -6,7 +6,7 @@
struct Foo(pub String);
macro_rules! foo {
($($t:tt)*) => (Foo(format!($($t)*)))
($($t:tt)*) => (Foo(format!($($t)*)))
}
fn main() {
@ -49,8 +49,8 @@ fn main() {
foo!("should not warn");
// Precision on string means slicing without panicking on size.
format!("{:.1}", "foo"); // could be `"foo"[..1]`
format!("{:.10}", "foo"); // could not be `"foo"[..10]`
format!("{:.1}", "foo"); // Could be `"foo"[..1]`
format!("{:.10}", "foo"); // Could not be `"foo"[..10]`
format!("{:.prec$}", "foo", prec = 1);
format!("{:.prec$}", "foo", prec = 10);