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

cfg_if!を削る #461

Closed
3 tasks done
qryxip opened this issue Apr 7, 2023 · 3 comments · Fixed by #709
Closed
3 tasks done

cfg_if!を削る #461

qryxip opened this issue Apr 7, 2023 · 3 comments · Fixed by #709

Comments

@qryxip
Copy link
Member

qryxip commented Apr 7, 2023

内容

cfg_if!を削ります。 #370 があるので、 #460 と同様にissueのみの作成です。

今このリポジトリにあるcfg_if!は、一つを除き #135 のときに導入されました。それらは

このようにすれば
diff --git a/crates/voicevox_core/src/publish.rs b/crates/voicevox_core/src/publish.rs
index 36d4316..aa11b32 100644
--- a/crates/voicevox_core/src/publish.rs
+++ b/crates/voicevox_core/src/publish.rs
@@ -47,13 +47,10 @@ impl VoicevoxCore {
             AccelerationMode::Auto => {
                 let supported_devices = SupportedDevices::get_supported_devices()?;
 
-                cfg_if! {
-                    if #[cfg(feature="directml")]{
-                        *supported_devices.dml()
-
-                    } else {
-                        *supported_devices.cuda()
-                    }
+                if cfg!(feature = "directml") {
+                    *supported_devices.dml()
+                } else {
+                    *supported_devices.cuda()
                 }
             }
             AccelerationMode::Cpu => false,
@@ -297,13 +294,11 @@ impl InferenceCore {
     fn can_support_gpu_feature(&self) -> Result<bool> {
         let supported_devices = SupportedDevices::get_supported_devices()?;
 
-        cfg_if! {
-            if #[cfg(feature = "directml")]{
-                Ok(*supported_devices.dml())
-            } else{
-                Ok(*supported_devices.cuda())
-            }
-        }
+        Ok(if cfg!(feature = "directml") {
+            *supported_devices.dml()
+        } else {
+            *supported_devices.cuda()
+        })
     }
     pub fn load_model(&mut self, speaker_id: u32) -> Result<()> {
         if self.initialized {
diff --git a/crates/voicevox_core/src/status.rs b/crates/voicevox_core/src/status.rs
index 309d060..eab14bf 100644
--- a/crates/voicevox_core/src/status.rs
+++ b/crates/voicevox_core/src/status.rs
@@ -15,11 +15,9 @@ use tracing::error;
 
 mod model_file;
 
-cfg_if! {
-    if #[cfg(not(feature="directml"))]{
-        use onnxruntime::CudaProviderOptions;
-    }
-}
+#[cfg(not(feature = "directml"))]
+use onnxruntime::CudaProviderOptions;
+
 use std::collections::{BTreeMap, BTreeSet};
 
 pub(crate) static MODEL_FILE_SET: Lazy<ModelFileSet> = Lazy::new(|| {
@@ -151,13 +149,11 @@ struct Style {
     id: u64,
 }
 static ENVIRONMENT: Lazy<Environment> = Lazy::new(|| {
-    cfg_if! {
-        if #[cfg(debug_assertions)]{
-            const LOGGING_LEVEL: LoggingLevel = LoggingLevel::Verbose;
-        } else{
-            const LOGGING_LEVEL: LoggingLevel = LoggingLevel::Warning;
-        }
-    }
+    const LOGGING_LEVEL: LoggingLevel = if cfg!(debug_assertions) {
+        LoggingLevel::Verbose
+    } else {
+        LoggingLevel::Warning
+    };
     Environment::builder()
         .with_name(env!("CARGO_PKG_NAME"))
         .with_log_level(LOGGING_LEVEL)
@@ -284,16 +280,18 @@ impl Status {
             .with_inter_op_num_threads(*session_options.cpu_num_threads() as i32)?;
 
         let session_builder = if *session_options.use_gpu() {
-            cfg_if! {
-                if #[cfg(feature = "directml")]{
-                    session_builder
-                        .with_disable_mem_pattern()?
-                        .with_execution_mode(onnxruntime::ExecutionMode::ORT_SEQUENTIAL)?
-                        .with_append_execution_provider_directml(0)?
-                } else {
-                    let options = CudaProviderOptions::default();
-                    session_builder.with_append_execution_provider_cuda(options)?
-                }
+            #[cfg(feature = "directml")]
+            {
+                session_builder
+                    .with_disable_mem_pattern()?
+                    .with_execution_mode(onnxruntime::ExecutionMode::ORT_SEQUENTIAL)?
+                    .with_append_execution_provider_directml(0)?
+            }
+
+            #[cfg(not(feature = "directml"))]
+            {
+                let options = CudaProviderOptions::default();
+                session_builder.with_append_execution_provider_cuda(options)?
             }
         } else {
             session_builder

このリポジトリから消すことができます。

session_builderのとこだけは少々不恰好になってしまうのでそこだけcfg_if!は残していいとは思うのですが、残りは使う理由も無いし、(cfg_if! { .. }の内側での)補完もRustfmtも効かなくなるので外してもいいと思いました。

(cfg-ifはgithub.com/rust-lang下で管理されている老舗でデファクトスタンダードなライブラリなので、「外部ライブラリへの依存を外す」という理由ではないです。私にとっては)

Pros 良くなる点

  • cfg_if!で囲っていた部分について、補完とRustfmtが効くようになる

Cons 悪くなる点

なし

実現方法

#370 マージ後、上記のようなことをする。

VOICEVOXのバージョン

HEAD

OSの種類/ディストリ/バージョン

  • Windows
  • macOS
  • Linux

その他

@qryxip qryxip added 機能向上 初心者歓迎タスク 初心者にも優しい簡単めなタスク labels Apr 7, 2023
@qryxip qryxip removed the 初心者歓迎タスク 初心者にも優しい簡単めなタスク label Apr 7, 2023
@Hiroshiba
Copy link
Member

とコンフリクトしちゃうのでとりあえずissueだけ作成ということでしょうか。
であれば了解です、issue作成ありがとうございます!

@qryxip
Copy link
Member Author

qryxip commented Apr 8, 2023

このissue、↑に初心者歓迎タスクを一瞬付けて消したのが見えると思うのですが、これは本文にcfg_if!の取り除き方を全部書いちゃったのでそれはどうなのかなと思ったのが消した理由です。

それで今気づいたのですが「VOICEVOXにとって重要じゃなくてかつしばらくの間は困らない問題」は初心者歓迎タスクの創出に使えばよいのかも。

@Hiroshiba
Copy link
Member

Hiroshiba commented Apr 8, 2023

まだ方針を決めてないのですが、前提知識がなくても結構進みやすいのが初心者歓迎タスクに合ってるのかなと思ってます。(その上そこそこ楽しいとなお良い)
cfg_ifはなかなか調べにくそうなライブラリの前提知識とボイボのビルド周りの知識が必要でかつ面白いかと言われると…って感じで、結構残っちゃいそうな雰囲気です。

でも簡単寄りではあると思うので、とりあえず付けておくというのはアリだと思います。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants