[SYCL] Cleanup the deprecated Intel attribute logic

[AaronBallman]

We previously sprinkled deprecated attribute logic around to a few places
when we could instead use a single helper function to do the work for us.

This also adds a fix-it hint when we find a deprecated attribute to help
the user convert to the new spelling.

As a drive-by, this fixes functions that were using Attr as a variable name.

[premanandrao]

Other than that question, the changes look good to me.

[premanandrao]

A question for you. One of the new developers had mentioned that they found it difficult to figure out what the single letter variable names meant in a function; and I think we had changed to using slightly longer spellings to make that clearer. What is the clang convention, if there is any, for this? Are single letter variable names considered acceptable in "obvious" situations?

[AaronBallman]

The community convention is to match whatever surrounding code is doing, to some extent. In the olden days, almost everything was given terrible acronym names like A for Attr, FD for FunctionDecl and whatnot, so older code often uses poorer names. Newer code will sometimes use more descriptive names, or sometimes continue with the poor conventions.

The community is fine accepting single-letter names when they're obvious or their scope is highly limited. I tend to use them still, but probably should break that habit. The only reason I was changing the names here is because Attr is also a type name and that sometimes makes for worse error reporting when modifying the code (you see this a lot with using Decl as a variable name too).

[srividya-sundaram]

Hi @AaronBallman , FYI, when I started contributing to this repo, @pvchupin pointed the Contribution-guide, which suggests a few pointers to be followed while preparing our patch ( See section "Prepare your patch"). One of them lists following the LLVM coding standards and for this specific case Naming-conventions lists how we could better name variables, functions etc.

class VehicleMaker {
  ...
Factory<Tire> F;               // Avoid: a non-descriptive abbreviation.
  Factory<Tire> Factory;      // Better: more descriptive.
  Factory<Tire> TireFactory;  // Even better: if VehicleMaker has more than one kind of factories.
};

I am not aware of cases where these guidelines are relaxed by the community, but thought I would just share.

[AaronBallman]

Thank you for the links @srividya-sundaram!

I am not aware of cases where these guidelines are relaxed by the community, but thought I would just share.

The relaxation I was talking about is in the Introduction of the coding standard document (https://llvm.org/docs/CodingStandards.html#introduction): "If you are extending, enhancing, or bug fixing already implemented code, use the style that is already being used so that the source is uniform and easy to follow."

That said, I agree that more descriptive names would be better in cases where the naming matters for understanding the code.

[srividya-sundaram]

If you are extending, enhancing, or bug fixing already implemented code, use the style that is already being used so that the source is uniform and easy to follow.

Thanks for pointing this out. I am myself in the midst of updating FE tests to be more readable/maintainable and found updating the non-descriptive variable names exhausting (made the code more understandable though IMO) . I could apply this rule for some cases I guess.

[AaronBallman]

FWIW, my usual rule of thumb is to use the ridiculously short names for variables and parameters that are only used in a limited scope, and go with more verbose names for member variables or within larger function body scopes.

People tend not to complain about code like:

if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
  FD->whatever();
}

or similarly restricted contexts. However, I also started back when the project used these short names more exclusively and so I picked up some bad habits. ;-)

[smanna12]

Looks good to me. Thanks for cleanup the logic.

[elizabethandrews]

LGTM Thanks!